Issue 5099207639695360: Made browser actions per tab on Safari (Closed)
|
Created:
Dec. 27, 2013, 7:22 p.m. by Sebastian Noack Modified:
Jan. 17, 2014, 3:15 p.m. Visibility:
Public. |
DescriptionThis patch set fixes two issues with browser actions on Safari:
* The title, icon and badge were set globally with Tab.browserAction.set*. So the badge was always set for the last loaded tab instead for the active tab.
* We mistakenly assumed that there can only be one toolbar item. But in fact there is one toolbar item per window, and we have to get the right one, in order to set its title, icon or badge.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebased, addressed comment and enabled to set global browser actions #
Total comments: 16
Patch Set 3 : Addressed comments #Patch Set 4 : Addressed comments #Patch Set 5 : Forgot to set tab in BrowserAction constructor #MessagesTotal messages: 10
http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... File safari/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:193: var tooltipItemPropertiesPerTab = new TabMap(); You meant toolbarItem, not tooltipItem, right? http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:203: } This should return something if no toolbar item is found - return null; at the end. The callers also need to handle this scenario properly. We *should* not see this scenario but I don't think the API guarantees that our toolbar item will be in every window. http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:258: } What's the point of globalTooltipItemProperties? I guess the idea is to show a proper state for new tabs where nothing loaded yet (no about:blank load notification in Safari?). But this should really be: toolbarItem.badge = 0; if (props) for (property in props) toolbarItem[property] = props[property]; This makes sure the badge is never stuck. As to the other properties, it should be fine to have those in a "wrong" state for new tabs (your approach doesn't really prevent it either).
Beside that I have added ext.browserAction (in additon to Tab.browserAction), in order to set global browser actions. The browser action title is set globally now. This is required, because there were some cases on Safari (e.g. on pages that can't be controlled by extensions) where the title wasn't set. Also since the title is the same on all pages it makes much more sense to set it globally, IMHO. http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... File safari/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:193: var tooltipItemPropertiesPerTab = new TabMap(); On 2014/01/15 15:24:47, Wladimir Palant wrote: > You meant toolbarItem, not tooltipItem, right? It is gone now. http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:203: } On 2014/01/15 15:24:47, Wladimir Palant wrote: > This should return something if no toolbar item is found - return null; at the > end. The callers also need to handle this scenario properly. We *should* not see > this scenario but I don't think the API guarantees that our toolbar item will be > in every window. Done. Even though I think that it is a bug when this actually happens, and I would prefer to see an error in that case. http://codereview.adblockplus.org/5099207639695360/diff/5629499534213120/safa... safari/ext/background.js:258: } On 2014/01/15 15:24:47, Wladimir Palant wrote: > What's the point of globalTooltipItemProperties? I guess the idea is to show a > proper state for new tabs where nothing loaded yet (no about:blank load > notification in Safari?). But this should really be: > > toolbarItem.badge = 0; > if (props) > for (property in props) > toolbarItem[property] = props[property]; > > This makes sure the badge is never stuck. As to the other properties, it should > be fine to have those in a "wrong" state for new tabs (your approach doesn't > really prevent it either). This approach just emulates the behavior we already have in Chrome, that when there is no state defined for the current tab, it falls back to the global state. You might be right that ABP relies on that behavior only for the badge. However I would like to keep the abstraction layer generic. So ABP-specific code should be avoided here. But even more important, having different behavior for Safari and Chrome, would quickly lead to strange bugs, when high level code changes. Also the code has changed a lot meanwhile. So I'm not sure whether this discussion is actually still current, since the new version can also set the global state via ext.browserAction, and it has to so, in order fix issues on Safari.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... background.js:413: ext.browserAction.setTitle(ext.i18n.getMessage("name")); If you take this route then there should also be a default image: ext.browserAction.setIcon("icons/abp-19.png"); Note however that for Chrome this is duplicating the defaults from manifest.json (or metadata.chrome). Ideally, the browser action implementation would just get the data from the metadata file. One way to implement that would be adding corresponding values to safariInfo.js.tmpl in buildtools. You probably won't have time to implement this right now, that's fine. We need at least a TODO comment on that however. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); This is unnecessary IMHO, ext.browserAction can be a dummy on Chrome - manifest.json already takes care of defining defaults. As mentioned already, I consider ext.browserAction to be merely an intermediate solution. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... File safari/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... safari/ext/background.js:192: var toolbarItemProperites = {}; toolbarItemProperites => toolbarItemProperties? http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... safari/ext/background.js:199: property = {global: safari.extension.toolbarItems[0][name], tabs: new TabMap()}; Will there always be at least one toolbar item? What if all Safari windows are closed? http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... safari/ext/background.js:266: if (toolbarItem && this._tab._tab == currentWindow.activeTab) Nit: It seems that |this._tab._tab == currentWindow.activeTab| should be checked first - before going through all the effort finding the right toolbar item.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... background.js:413: ext.browserAction.setTitle(ext.i18n.getMessage("name")); On 2014/01/16 09:10:05, Wladimir Palant wrote: > If you take this route then there should also be a default image: > > ext.browserAction.setIcon("icons/abp-19.png"); > > Note however that for Chrome this is duplicating the defaults from manifest.json > (or metadata.chrome). Ideally, the browser action implementation would just get > the data from the metadata file. One way to implement that would be adding > corresponding values to safariInfo.js.tmpl in buildtools. > > You probably won't have time to implement this right now, that's fine. We need > at least a TODO comment on that however. I think it is fine as it is. the default icon is also defined in Info.plist on Safari. The reason we have to set the title in the code, is to have it translated. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); On 2014/01/16 09:10:05, Wladimir Palant wrote: > This is unnecessary IMHO, ext.browserAction can be a dummy on Chrome - > manifest.json already takes care of defining defaults. As mentioned already, I > consider ext.browserAction to be merely an intermediate solution. I think it is a pretty bad idea to have different behavior for Chrome and Safari. Also it makes the API more complete, to be able to set browser actions per tab as well as globally. And the code required therefore is minimal for Chrome. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... File safari/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... safari/ext/background.js:192: var toolbarItemProperites = {}; On 2014/01/16 09:10:05, Wladimir Palant wrote: > toolbarItemProperites => toolbarItemProperties? Done. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/safa... safari/ext/background.js:199: property = {global: safari.extension.toolbarItems[0][name], tabs: new TabMap()}; On 2014/01/16 09:10:05, Wladimir Palant wrote: > Will there always be at least one toolbar item? What if all Safari windows are > closed? Done.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... background.js:413: ext.browserAction.setTitle(ext.i18n.getMessage("name")); On 2014/01/16 12:45:02, Sebastian Noack wrote: > I think it is fine as it is. the default icon is also defined in Info.plist on > Safari. The reason we have to set the title in the code, is to have it > translated. The difference however is: Chrome will reset the icon automatically for a new tab whereas Safari won't. Safari considers toolbar items to be per-window rather than per-tab so any per-tab behavior needs to be defined explicitly. Also, there is no reason to change title programmatically in Chrome - the title is translated already. manifest.json specifies __MSG_name__ rather than an explicit string. So the functionality you are working on is only required for Safari and I see little reason to change behavior in Chrome. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); On 2014/01/16 12:45:02, Sebastian Noack wrote: > I think it is a pretty bad idea to have different behavior for Chrome and > Safari. Also it makes the API more complete, to be able to set browser actions > per tab as well as globally. And the code required therefore is minimal for > Chrome. It obscures the logic even more than it already is, for a functionality that is completely unnecessary in Chrome.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... background.js:413: ext.browserAction.setTitle(ext.i18n.getMessage("name")); On 2014/01/16 12:55:18, Wladimir Palant wrote: > The difference however is: Chrome will reset the icon automatically for a new > tab whereas Safari won't. Safari considers toolbar items to be per-window rather > than per-tab so any per-tab behavior needs to be defined explicitly. Yes, but we backup the default (which comes from Info.plist) before setting any per tab browser action. So it is restored, without having the default redundant in the javascript code. > Also, there is no reason to change title programmatically in Chrome - the title > is translated already. manifest.json specifies __MSG_name__ rather than an > explicit string. So the functionality you are working on is only required for > Safari and I see little reason to change behavior in Chrome. Yes, but it won't hurt to set the title programmatically on Chrome. In fact we already did that before. The only thing I changed, is to set the title globally instead of everytime for every tab. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); On 2014/01/16 12:55:18, Wladimir Palant wrote: > On 2014/01/16 12:45:02, Sebastian Noack wrote: > > I think it is a pretty bad idea to have different behavior for Chrome and > > Safari. Also it makes the API more complete, to be able to set browser actions > > per tab as well as globally. And the code required therefore is minimal for > > Chrome. > > It obscures the logic even more than it already is, for a functionality that is > completely unnecessary in Chrome. I don't agree. It isn't that much code, I have added. And in order to make the API consistent between Chrome and Safari, I think it is totally worth it. And even if we wouldn't expose ext.browserAction, having the tab specific code moved into a separate class makes the code slightly better organized, IMHO.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/back... background.js:413: ext.browserAction.setTitle(ext.i18n.getMessage("name")); On 2014/01/16 14:01:52, Sebastian Noack wrote: > Yes, but we backup the default (which comes from Info.plist) before setting any > per tab browser action. So it is restored, without having the default redundant > in the javascript code. Ok, I see - setting the image explicitly is indeed unnecessary. > Yes, but it won't hurt to set the title programmatically on Chrome. In fact we > already did that before. The only thing I changed, is to set the title globally > instead of everytime for every tab. No, setting the title programmatically won't hurt - as long as doing so doesn't require adding one more class to the class hierarchy and introducing a concept of global properties in addition to per-tab properties. http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); On 2014/01/16 14:01:52, Sebastian Noack wrote: > I don't agree. It isn't that much code, I have added. And in order to make the > API consistent between Chrome and Safari, I think it is totally worth it. > > And even if we wouldn't expose ext.browserAction, having the tab specific code > moved into a separate class makes the code slightly better organized, IMHO. Quite frankly, I disagree. IMHO the abstraction layer is already bordering to overarchitecturing. Introducing an additional concept and extending the class hierarchy further for no good reason isn't something I will approve. As it is now, browser actions are *always* per-tab - we copied that concept from Chrome and it is good that way. There is merely a default for the tabs where nothing has been set yet. We shouldn't muddy the waters unnecessarily.
http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/5099207639695360/diff/5639274879778816/chro... chrome/ext/background.js:541: ext.browserAction = new BrowserAction(); My main concern is having different behavior for Safari and Chrome. But the only reason we would need a global browser action API is to have the extension name translated in the toolbar item's label and tooltip on Safari. However considering that there is currently only one language (Arabic) where we actually translate the extension name and I'm not sure whether that is even intended, I think the solution isn't worth all that extra code (even when we implement it only for Safari) and the discussion about it. And considering that there are already many other places in Safari where we just can't translate the extension name (e.g. under Preferences->Extensions and in the "Customize Toolbar" menu), it might even be preferable to have the extension name consistent and therefore don't translate it here too. I have uploaded a new patchset, with the code for configuring browser actions globally removed, completly (also for Safari). So you get a simpler implementation. And I get a consistent API between Chrome and Safari.
LGTM 
|
