Fix #76: Live update popup with extracted product while open #115

biancadanforth · 2018-09-14T23:09:37Z

Previously, the extracted product information was only passed by changing the popup's URL, which would only update on closing and re-opening the popup.

Now, when a product is detected on a page, the background script sends a message with extracted product information to the browserAction script, if available, to update and re-render the browserAction popup. This makes it possible for the popup to update while remaining open.

biancadanforth · 2018-09-14T23:12:16Z

@Osmose Ready for your review. Perhaps there's a better way to do this, but this seemed to work cleanly.

Osmose

Nice work! You made this much simpler than I imagined it'd be.

Osmose · 2018-09-17T17:35:57Z

src/background/index.js

@@ -48,6 +49,13 @@ function handleExtractedProductData(extractedProduct, sender) {
    });
  }

+  // Update the toolbar popup while it is open with the current page's product
+  browser.runtime.sendMessage({
+    from: 'background',


Why the from? Isn't the subject enough to identify the event?

Osmose · 2018-09-17T17:44:00Z

src/browser_action/index.jsx

-  document.getElementById('browser-action-app'),
-);
+browser.runtime.onMessage.addListener((message) => {
+  if (message.from === 'background' && message.subject === 'extracted-product') {


This listener doesn't check to see whether the popup that is currently open matches the tab that a product was extracted from.

For example, if you open a product on Amazon, Cmd+Click to open a new tab to a different product without leaving the current tab, and open the popup before the new tab finishes loading, the popup is replaced with the new product because extraction finished and it isn't checking which tab the current popup is assigned to.

Osmose · 2018-09-17T17:47:48Z

src/background/index.js

@@ -37,7 +37,8 @@ function handleExtractedProductData(extractedProduct, sender) {
    return;
  }

-  // Update the toolbar icon's URL with the current page's product if we can
+  // Update the toolbar popup the next time it is opened with the current page's product
+  // This is done by passing product information via the popup's URL


I don't know that we need to call out that the data is passed via URL when the code immediately below isn't unclear about that fact. Too much commenting makes code harder to read; in general we want to comment things that aren't obvious from the code or otherwise helpful during a skim.

Osmose · 2018-09-17T17:49:28Z

src/browser_action/index.jsx

+  }
+});
+
+function render() {


This render function working with a global variable is a bit hard to follow.

What if BrowserActionApp just stores the initially extracted product in its state, and then it registers the listener and updates the state instead of re-running the render method?

Previously, the extracted product information was only passed by changing the popup's URL, which would only update on closing and re-opening the popup. Now, when a product is detected on a page, the background script sends a message with extracted product information to the browserAction script, if available, to update and re-render the browserAction popup. This makes it possible for the popup to update while remaining open.

* Add check to ensure extracted product card is only displayed when the tab it was extracted from is the currently active tab in the currently active window. * Note: The use of the `windowTypes` key on the `getInfo` object that can be passed into `browser.windows.getCurrent` has been [deprecated(https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/parent/ext-tabs-base.js#843). * Store extracted product info for the current page as state in 'BrowserActionApp' component, updating that state when a message from the background script is received. This is easier to follow than re-calling the ReactDOM.render code in './src/browser_action/index.jsx'.

biancadanforth · 2018-09-18T23:17:18Z

@Osmose Ready for a re-review! Thanks for the comments.

Osmose

r+wc

Osmose · 2018-09-19T17:19:07Z

src/background/index.js

-    subject: 'extracted-product',
-    data: extractedProduct,
-  });
+  if (sender.tab.active && await isWindowActive(sender.tab.windowId)) {


I'm a bit confused as to why this works, because we're using the window ID, which should still fail for two tabs with different products in the same window. But I can't get it to fail! Weird. Great work?

In any case, checking the ID before sending the message is finnicky; message sends are asynchronous, and the popup could change in the meantime. While it's unlikely, it's generally better to check on the receiving end. You can pass the tabId in the URL for the panel, and include the tabId in this message so that the receiving end can compare.

I'm a bit confused as to why this works, because we're using the window ID, which should still fail for two tabs with different products in the same window. But I can't get it to fail! Weird. Great work?

Unless I'm misunderstanding you, while both tabs have the same window, only one tab in that window would be active, so the first condition would catch that situation.

In any case, checking the ID before sending the message is finnicky; message sends are asynchronous, and the popup could change in the meantime. While it's unlikely, it's generally better to check on the receiving end. You can pass the tabId in the URL for the panel, and include the tabId in this message so that the receiving end can compare.

@Osmose , wouldn't I want to check both tabId and windowId on the receiving end? It sounds like you are suggesting only moving the tabId...

The suggested approach was to pass the tabId as a URL search parameter in a similar fashion to how we pass the initial extractedProduct information from the background script to the browserAction script, passing it into the top level component as props.

Ultimately, we decided we would leave it as- is due to the following:

The browserAction URL does not update until the popup is closed and reopened, so the tabId cannot be passed via the URL for live updating the popup.

The browserAction context cannot directly obtain the current active tabId.

An alternative would be to change the browserAction base URL so that it always has the tabID included. This would make it hard to test, since you would need dependency injection (i.e. we would have to mock the tabId or set it as global state and then remember to unset once the test is done rather than just setting it to whatever we want as props).

Osmose · 2018-09-19T17:24:40Z

src/background/index.js

+  if (sender.tab.active && await isWindowActive(sender.tab.windowId)) {
+    browser.runtime.sendMessage({
+      subject: 'extracted-product',
+      data: extractedProduct,


Can we just name this key extractedProduct? There's not rules on the shape of the message and there's not much reason to be generic about any keys besides the subject one.

* Remove the windowId check when updating the popup live, since the popup will only ever be open in the active window * Move the live update code block inside of the 'sender.tab' check in './src/background/index.js', since the code also executes when background extraction succeeds, where no 'sender.tab' exists. * Keep the active tabId check in the background.js script rather than in the browserAction script receiving end because: * The browserAction URL does not update until the popup is closed and reopened, so the tabId cannot be passed via the URL for live updating the popup. * The browserAction context [cannot directly obtain](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/getCurrent) the current active `tabId`. * An alternative would be to change the browserAction base URL so that it always has the tabID included. This would make it hard to test, since you would need dependency injection (i.e. we would have to mock the tabId or set it as global state and then remember to unset it once the test is done rather than just setting it to whatever we want as props).

biancadanforth requested a review from Osmose September 14, 2018 23:09

Osmose suggested changes Sep 17, 2018

View reviewed changes

biancadanforth added 2 commits September 18, 2018 15:23

biancadanforth force-pushed the 76-live-update-popup branch from 7ddb4eb to 2ed82c0 Compare September 18, 2018 23:14

biancadanforth requested a review from Osmose September 18, 2018 23:17

Osmose approved these changes Sep 19, 2018

View reviewed changes

biancadanforth force-pushed the 76-live-update-popup branch from f65d903 to a9589ef Compare September 19, 2018 22:06

biancadanforth force-pushed the 76-live-update-popup branch from a9589ef to 0c69b1c Compare September 19, 2018 22:08

biancadanforth merged commit e127e29 into master Sep 19, 2018

biancadanforth deleted the 76-live-update-popup branch September 19, 2018 22:11

biancadanforth mentioned this pull request Sep 20, 2018

Fix #42: Improve price string parsing. #111

Merged

biancadanforth restored the 76-live-update-popup branch September 20, 2018 16:36

biancadanforth deleted the 76-live-update-popup branch September 20, 2018 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #76: Live update popup with extracted product while open #115

Fix #76: Live update popup with extracted product while open #115

biancadanforth commented Sep 14, 2018 •

edited

Loading

biancadanforth commented Sep 14, 2018

Osmose left a comment

Osmose Sep 17, 2018

Osmose Sep 17, 2018

Osmose Sep 17, 2018

Osmose Sep 17, 2018

biancadanforth commented Sep 18, 2018

Osmose left a comment

Osmose Sep 19, 2018

biancadanforth Sep 19, 2018

biancadanforth Sep 19, 2018

Osmose Sep 19, 2018

Fix #76: Live update popup with extracted product while open #115

Fix #76: Live update popup with extracted product while open #115

Conversation

biancadanforth commented Sep 14, 2018 • edited Loading

biancadanforth commented Sep 14, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Sep 18, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Sep 14, 2018 •

edited

Loading