Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
415813003:
Improve extension icon prediction (Closed)
DescriptionImprove extension icon prediction
BUG=394920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289062
Patch Set 1 #
Total comments: 11
Patch Set 2 : Minor changes #
Total comments: 11
Patch Set 3 : Minor changes #Patch Set 4 : Enhanced GetBestFitAction, implement in ActiveScriptController #Patch Set 5 : Pointer stuff #
Total comments: 12
Patch Set 6 : Added tests, modified GetBestFitAction #
Total comments: 12
Patch Set 7 : Enhanced tests with callback #Patch Set 8 : Updated comments, buildExtension parameters #
Total comments: 12
Patch Set 9 : Enhanced tests, updated icon logic #
Total comments: 26
Patch Set 10 : Simplify icon logic, clean up unit tests #
Total comments: 17
Patch Set 11 : Minor changes #
Total comments: 5
Patch Set 12 : Moar minor changes #Patch Set 13 : Fixed null check that was causing browser test failure #Patch Set 14 : Fixed unittest to add extensions to registry #
Total comments: 7
Patch Set 15 : Final minor changes #
Messages
Total messages: 69 (0 generated)
How's this look for a start? PopulateMissingValues will look for missing icons from the ExtensionAction and add in the path for the largest icon from the extension's "icons" manifest key. Work needs to be done in the Active Script Controller. It creates a whole new action, so this code doesn't get run for it. We'll have to think of a good way to work around this, but I figured I'd show you what I've got so far.
nice! a couple of comments, didn't have time to look into this, but let's actually use this logic now in the active script controller stuff. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:89: void PopulateMissingValues(const extensions::Extension& extension, extensions:: unnecessary (you might as well do file-wide replacement) https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:91: if (action) { if anything, early-return here if it's null. but I think it's better to make sure that it's only ever passed a non-NULL |action|. it's weird to pass in a NULL out-param. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:118: make_scoped_ptr(new ExtensionIconSet(new_default_icon))); to avoid all this copying you could assign new_default_icon to a scoped_ptr right from the start. could you also clean up that slightly awkward new_default_icon thing as well? scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet()); if (action->default_icon()) *default_icon = *action->default_icon(); https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:150: action.get()); nice! but let's be conservative for now and just do this on GetBestFitAction. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:176: else no else after return.
I'll start looking into substitution for GetBestFitAction. What exactly is the use-case for this? It seems like in a lot of cases, the caller specifically looks for either a page or browser action. The ActiveScriptController retrieves ActionInfo in order to set up a new page action; it doesn't call GetPageAction or GetBrowserAction. Besides, don't script injection requests always show up as page actions? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:89: void PopulateMissingValues(const extensions::Extension& extension, On 2014/07/24 00:41:26, kalman wrote: > extensions:: unnecessary (you might as well do file-wide replacement) Done. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:91: if (action) { On 2014/07/24 00:41:26, kalman wrote: > if anything, early-return here if it's null. but I think it's better to make > sure that it's only ever passed a non-NULL |action|. it's weird to pass in a > NULL out-param. Done. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:118: make_scoped_ptr(new ExtensionIconSet(new_default_icon))); On 2014/07/24 00:41:26, kalman wrote: > to avoid all this copying you could assign new_default_icon to a scoped_ptr > right from the start. could you also clean up that slightly awkward > new_default_icon thing as well? > > scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet()); > if (action->default_icon()) > *default_icon = *action->default_icon(); Done. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:150: action.get()); On 2014/07/24 00:41:26, kalman wrote: > nice! but let's be conservative for now and just do this on GetBestFitAction. Unfortunately, this needs to be done upon creation of the action, otherwise the browser crashes via segfault upon reloading of an unpacked extension. It took me a while to figure out why it wasn't working, but I believe it has to do with the fact that the default_icon is used for creation of the IconFactory when action views are updated. If we populate these values after the action has been used to create the icon factory, theres a discrepancy between the default_icon used for the factory and the one now attributed to the action. However, if we populate these values immediately after the action is created, and before its used to create the factory, this doesn't happen, since reloading an extension updates all the action views, which recreates all the icon factories. Relevant code: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... The action's default_icon is passed in via reference, and when we change that reference in the ExtensionAction in PopulateMissingValues, bad things happen. Is there any particular reason you don't want to make this call upon creation? It seemed appropriate to update all of this before the ExtensionAction lets loose its references, and the only way this call could do anything different the second time is if the extension is reloaded, in which case it's recreated anyway. https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:176: else On 2014/07/24 00:41:26, kalman wrote: > no else after return. Done.
https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_action_manager.cc:150: action.get()); On 2014/07/24 17:57:45, gpdavis wrote: > On 2014/07/24 00:41:26, kalman wrote: > > nice! but let's be conservative for now and just do this on GetBestFitAction. > > Unfortunately, this needs to be done upon creation of the action, otherwise the > browser crashes via segfault upon reloading of an unpacked extension. It took > me a while to figure out why it wasn't working, but I believe it has to do with > the fact that the default_icon is used for creation of the IconFactory when > action views are updated. If we populate these values after the action has been > used to create the icon factory, theres a discrepancy between the default_icon > used for the factory and the one now attributed to the action. However, if we > populate these values immediately after the action is created, and before its > used to create the factory, this doesn't happen, since reloading an extension > updates all the action views, which recreates all the icon factories. > > Relevant code: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... > > The action's default_icon is passed in via reference, and when we change that > reference in the ExtensionAction in PopulateMissingValues, bad things happen. > > Is there any particular reason you don't want to make this call upon creation? > It seemed appropriate to update all of this before the ExtensionAction lets > loose its references, and the only way this call could do anything different the > second time is if the extension is reloaded, in which case it's recreated > anyway. I think that's fine.
On 2014/07/24 17:57:46, gpdavis wrote: > I'll start looking into substitution for GetBestFitAction. What exactly is the > use-case for this? It seems like in a lot of cases, the caller specifically > looks for either a page or browser action. The ActiveScriptController retrieves > ActionInfo in order to set up a new page action; it doesn't call GetPageAction > or GetBrowserAction. Besides, don't script injection requests always show up as > page actions? > The use case is whenever we show an icon for an extension that isn't explicit. Page and browser actions are explicit; active script controller isn't, because an icon might show up for an extension that has neither a browser nor page action.
On 2014/07/24 18:02:53, kalman wrote: > On 2014/07/24 17:57:46, gpdavis wrote: > > I'll start looking into substitution for GetBestFitAction. What exactly is > the > > use-case for this? It seems like in a lot of cases, the caller specifically > > looks for either a page or browser action. The ActiveScriptController > retrieves > > ActionInfo in order to set up a new page action; it doesn't call GetPageAction > > or GetBrowserAction. Besides, don't script injection requests always show up > as > > page actions? > > > > The use case is whenever we show an icon for an extension that isn't explicit. > Page and browser actions are explicit; active script controller isn't, because > an icon might show up for an extension that has neither a browser nor page > action. Okay, that makes sense. But in the ActiveScriptController, it's action info that's retrieved, not the action itself, and the action info doesn't live in the action.
ActiveScriptController is Devlin's territory - could you have a quick look at that?
https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:87: // Loads resources missing from |action| (ie title, icons) from "icons" key of "i.e." "the "icons" key" https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:91: if (!action) Why are we passing in NULL actions in the first place? https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:148: PopulateMissingValues(*service->GetExtensionById(extension_id, false), this is deprecated. Use ExtensionRegistry::enabled_extensions().GetByID(). https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:171: ExtensionAction* ExtensionActionManager::GetBestFitAction( I'm a bit confused. I thought part of the purpose here was to be able to _get an icon for an extension that didn't declare a page action or a browser action_. But here, we're just gonna return NULL if it doesn't have one of those two?
@kalman, can you clarify the bit about the purpose of GetBestFitAction? If no action is found, should it construct one and pull as much information from the manifest as possible? https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:87: // Loads resources missing from |action| (ie title, icons) from "icons" key of On 2014/07/25 16:46:55, Devlin wrote: > "i.e." > "the "icons" key" Done. https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:91: if (!action) On 2014/07/25 16:46:55, Devlin wrote: > Why are we passing in NULL actions in the first place? You're right; we shouldn't need to check for this. I had left it in since I was moving this function call around, but as it sits now, it should never get a NULL action. Done. https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:148: PopulateMissingValues(*service->GetExtensionById(extension_id, false), On 2014/07/25 16:46:55, Devlin wrote: > this is deprecated. Use ExtensionRegistry::enabled_extensions().GetByID(). Any idea why this deprecation isn't documented? Done. https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:171: ExtensionAction* ExtensionActionManager::GetBestFitAction( On 2014/07/25 16:46:55, Devlin wrote: > I'm a bit confused. I thought part of the purpose here was to be able to _get > an icon for an extension that didn't declare a page action or a browser action_. > But here, we're just gonna return NULL if it doesn't have one of those two? I'm actually a bit confused about the purpose of this as well. Should this be constructing an action and populating it with icons from the "icons" manifest key if a page/browser action is not found?
https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:148: PopulateMissingValues(*service->GetExtensionById(extension_id, false), On 2014/07/28 21:40:36, gpdavis wrote: > On 2014/07/25 16:46:55, Devlin wrote: > > this is deprecated. Use ExtensionRegistry::enabled_extensions().GetByID(). > > Any idea why this deprecation isn't documented? Done. good point: https://codereview.chromium.org/426553010/ https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:171: ExtensionAction* ExtensionActionManager::GetBestFitAction( On 2014/07/28 21:40:36, gpdavis wrote: > On 2014/07/25 16:46:55, Devlin wrote: > > I'm a bit confused. I thought part of the purpose here was to be able to _get > > an icon for an extension that didn't declare a page action or a browser > action_. > > But here, we're just gonna return NULL if it doesn't have one of those two? > > I'm actually a bit confused about the purpose of this as well. Should this be > constructing an action and populating it with icons from the "icons" manifest > key if a page/browser action is not found? Yes exactly. one could imagine super-cool functionality here, like generating its icon from the first letter of the extension's name or something. but for now... it should try to get icons from the extension's icons, yes.
https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:171: ExtensionAction* ExtensionActionManager::GetBestFitAction( On 2014/07/28 21:55:36, kalman wrote: > On 2014/07/28 21:40:36, gpdavis wrote: > > On 2014/07/25 16:46:55, Devlin wrote: > > > I'm a bit confused. I thought part of the purpose here was to be able to > _get > > > an icon for an extension that didn't declare a page action or a browser > > action_. > > > But here, we're just gonna return NULL if it doesn't have one of those two? > > > > I'm actually a bit confused about the purpose of this as well. Should this be > > constructing an action and populating it with icons from the "icons" manifest > > key if a page/browser action is not found? > > Yes exactly. > > one could imagine super-cool functionality here, like generating its icon from > the first letter of the extension's name or something. > > but for now... it should try to get icons from the extension's icons, yes. Can do. A couple of design questions: Script injection requests are always supposed to end up as page actions, right? My initial inclination was that GetBestFitAction would first try GetBrowserAction, then GetPageAction, then fallback to creating a new action. First of all, should the returned action be used directly, or just for reference? For example, the ActiveScriptController creates a new page action and sets its default icons manually. Instead, should it take the generated action and run with it, or should it only use it for reference for the new action it's creating? Follow up: if we do indeed use the action returned by GetBestFitAction, what if the extension has a browser action and it requests script injection permissions? Then the returned action would be a browser action. Finally, in regards to pointers, should I be using a scoped pointer within this function, then use release() to return it? If a new action is created, it's not going to be stored anywhere within the ExtensionActionManager.
On 2014/07/29 00:18:02, gpdavis wrote: > https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... > File chrome/browser/extensions/extension_action_manager.cc (right): > > https://codereview.chromium.org/415813003/diff/20001/chrome/browser/extension... > chrome/browser/extensions/extension_action_manager.cc:171: ExtensionAction* > ExtensionActionManager::GetBestFitAction( > On 2014/07/28 21:55:36, kalman wrote: > > On 2014/07/28 21:40:36, gpdavis wrote: > > > On 2014/07/25 16:46:55, Devlin wrote: > > > > I'm a bit confused. I thought part of the purpose here was to be able to > > _get > > > > an icon for an extension that didn't declare a page action or a browser > > > action_. > > > > But here, we're just gonna return NULL if it doesn't have one of those > two? > > > > > > I'm actually a bit confused about the purpose of this as well. Should this > be > > > constructing an action and populating it with icons from the "icons" > manifest > > > key if a page/browser action is not found? > > > > Yes exactly. > > > > one could imagine super-cool functionality here, like generating its icon from > > the first letter of the extension's name or something. > > > > but for now... it should try to get icons from the extension's icons, yes. > > Can do. A couple of design questions: > > Script injection requests are always supposed to end up as page actions, right? Not necessarily, and in fact we're going to start generating a browser action for every extension. This code will be especially important. > My initial inclination was that GetBestFitAction would first try > GetBrowserAction, then GetPageAction, then fallback to creating a new action. Whether you try browser or page actions first is somewhat moot, since extensions are only allowed to set one or the other. > > First of all, should the returned action be used directly, or just for > reference? For example, the ActiveScriptController creates a new page action > and sets its default icons manually. Instead, should it take the generated > action and run with it, or should it only use it for reference for the new > action it's creating? I don't really understand this question... what does "just for reference" mean? > > Follow up: if we do indeed use the action returned by GetBestFitAction, what if > the extension has a browser action and it requests script injection permissions? > Then the returned action would be a browser action. Not 100% sure what you're asking here but yes if an extension has a browser action then you'd expect the script thing to use the same configuration as the browser action. > > Finally, in regards to pointers, should I be using a scoped pointer within this > function, then use release() to return it? If a new action is created, it's not > going to be stored anywhere within the ExtensionActionManager. If you're transferring ownership from one entity to another you can use scoped_ptr::Pass(), like scoped_ptr<ExtensionAction> action = ...; ... return action.Pass();
On 2014/07/29 00:23:42, kalman wrote: > Not necessarily, and in fact we're going to start generating a browser action > for every extension. This code will be especially important. Oh, okay, well then that changes things. So if the extension has any action at all, we can return it and use it for the request? > > First of all, should the returned action be used directly, or just for > > reference? For example, the ActiveScriptController creates a new page action > > and sets its default icons manually. Instead, should it take the generated > > action and run with it, or should it only use it for reference for the new > > action it's creating? > > I don't really understand this question... what does "just for reference" mean? The ActiveScriptController creates a new action and uses any ActionInfo it can find to populate it. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... This question is kind of irrelevant now that I know it's OK to put an injection request in a browser action. But I was essentially asking whether we should use the returned extension directly, or just pull information from it (since I was under the impression that script injection requests are only to be placed in page actions, and GetBestFitAction has the potential to return a browser action). > > Finally, in regards to pointers, should I be using a scoped pointer within > this > > function, then use release() to return it? If a new action is created, it's > not > > going to be stored anywhere within the ExtensionActionManager. > > If you're transferring ownership from one entity to another you can use > scoped_ptr::Pass(), like > > scoped_ptr<ExtensionAction> action = ...; > ... > return action.Pass(); Ah, okay. Will use Pass instead of release.
The ActiveScriptController uses linked_ptrs, so I released the scoped_ptr return value after the GetBestFitAction call. Is there a preferred pointer type to return for this method?
On 2014/07/29 00:33:45, gpdavis wrote: > On 2014/07/29 00:23:42, kalman wrote: > > Not necessarily, and in fact we're going to start generating a browser action > > for every extension. This code will be especially important. > > Oh, okay, well then that changes things. So if the extension has any action at > all, we can return it and use it for the request? Yep that's a good place to start. It's possible to create incomplete browser/page actions though - I'm not sure if we try to populate these with missing fields elsewhere. If we don't, we probably should, and this "GetBestFit" function would be somewhat orthogonal (though possibly share code). > > > > > First of all, should the returned action be used directly, or just for > > > reference? For example, the ActiveScriptController creates a new page > action > > > and sets its default icons manually. Instead, should it take the generated > > > action and run with it, or should it only use it for reference for the new > > > action it's creating? > > > > I don't really understand this question... what does "just for reference" > mean? > > The ActiveScriptController creates a new action and uses any ActionInfo it can > find to populate it. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > This question is kind of irrelevant now that I know it's OK to put an injection > request in a browser action. But I was essentially asking whether we should use > the returned extension directly, or just pull information from it (since I was > under the impression that script injection requests are only to be placed in > page actions, and GetBestFitAction has the potential to return a browser > action). Still not quite sure what you're asking. If it's that... is it ok to create a new ActionInfo or whatever each time, then yeah, it's pretty cheap so sure why not.
On 2014/07/29 00:56:58, gpdavis wrote: > The ActiveScriptController uses linked_ptrs, so I released the scoped_ptr return > value after the GetBestFitAction call. Is there a preferred pointer type to > return for this method? The main reason to use linked_ptr - and perhaps the only compelling reason - is to put non-copyable types in collections. This is what ActiveScriptController uses them for: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... so that's what you're seeing there. It's very rare to see linked_ptrs part of a class' public interface, whether that be as an input or an output. They're almost always implementation details of classes.
On 2014/07/29 14:45:57, kalman wrote: > On 2014/07/29 00:33:45, gpdavis wrote: > > On 2014/07/29 00:23:42, kalman wrote: > > > Not necessarily, and in fact we're going to start generating a browser > action > > > for every extension. This code will be especially important. > > > > Oh, okay, well then that changes things. So if the extension has any action > at > > all, we can return it and use it for the request? > > Yep that's a good place to start. It's possible to create incomplete > browser/page actions though - I'm not sure if we try to populate these with > missing fields elsewhere. If we don't, we probably should, and this "GetBestFit" > function would be somewhat orthogonal (though possibly share code). > > > > > > > > > First of all, should the returned action be used directly, or just for > > > > reference? For example, the ActiveScriptController creates a new page > > action > > > > and sets its default icons manually. Instead, should it take the > generated > > > > action and run with it, or should it only use it for reference for the new > > > > action it's creating? > > > > > > I don't really understand this question... what does "just for reference" > > mean? > > > > The ActiveScriptController creates a new action and uses any ActionInfo it can > > find to populate it. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > This question is kind of irrelevant now that I know it's OK to put an > injection > > request in a browser action. But I was essentially asking whether we should > use > > the returned extension directly, or just pull information from it (since I was > > under the impression that script injection requests are only to be placed in > > page actions, and GetBestFitAction has the potential to return a browser > > action). > > Still not quite sure what you're asking. If it's that... is it ok to create a > new ActionInfo or whatever each time, then yeah, it's pretty cheap so sure why > not. The ActiveScriptController creates a new action for the injection request regardless of whether the extension already has an action: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... If we use GetBestFitAction here, an ExtensionAction* will be returned. My question was, are we going to use this ExtensionAction directly, or are we going to use it to gather information for the creation of a new page action? It looks like currently, the location bar controller asks the ActiveScriptController for an extension action if it doesn't find a page action for a given extension. We wouldn't want to return a browser action here, would we? You said we're going to start adding browser actions for every extension-- is this even worth worrying about at the moment? I tested with an extension that has a browser action, and upon injection request, it created a page action alongside the browser action. Clicking the browser action did make the page action disappear, and granted permission. This seems like odd behavior. I guess my question is what are we expecting to happen if an extension with an action requests injection? If the extension has a browser action, do we want a page action to show up alongside it so the user can see that something has changed? Otherwise, it wouldn't be obvious that the extension is requesting permission to inject a script. Same goes for an extension with a page action; if there's already a page action, how will the user know about the permissions request? Should we be adding a second page action?
the code you have is basically right. there are a couple of different intersecting concepts here: (1) page and browser actions are APIs that an extension can interact with. they can request that the page and browser actions have a particular icon etc. this is distinct from: (2) in some places we generate an icon that represents the extension and show it somewhere, like in the omnibox API or as a favicon or whatever. in the case of (2) as of fairly recently another place we show these generated icons on the right hand side of the omnibox, when an extension is executing a script. it also so happens that we show page actions there. unfortunately, the logic is still tied together too much such that these icons we show in the toolbar are based on the page/browser actions that the extension has asked for, *not* necessarily a complete representation of them. also in the case of (2) we're going to start showing these generated icons in the toolbar, which is where we happen to show browser actions. this is more complicated: we will want to show the browser action here if it's available. if not, use the page action. if not, generate one. and this is basically what you've written.
On 2014/08/01 00:09:34, kalman wrote: > the code you have is basically right. > > there are a couple of different intersecting concepts here: > > (1) page and browser actions are APIs that an extension can interact with. they > can request that the page and browser actions have a particular icon etc. > > this is distinct from: > > (2) in some places we generate an icon that represents the extension and show it > somewhere, like in the omnibox API or as a favicon or whatever. > > in the case of (2) as of fairly recently another place we show these generated > icons on the right hand side of the omnibox, when an extension is executing a > script. it also so happens that we show page actions there. unfortunately, the > logic is still tied together too much such that these icons we show in the > toolbar are based on the page/browser actions that the extension has asked for, > *not* necessarily a complete representation of them. > > also in the case of (2) we're going to start showing these generated icons in > the toolbar, which is where we happen to show browser actions. this is more > complicated: we will want to show the browser action here if it's available. if > not, use the page action. if not, generate one. > > and this is basically what you've written. Okay, I see. So then we're cool with what I've done here? I do want to ask about SetTitle and SetIsVisible as well: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... We shouldn't need to set the title, since actions will populate missing titles from the manifest. As far as setting IsVisible to true goes, won't there be any conflicts if the extension already has a page action? Say we have a page action that's visible, and the extension requests a script injection. When the permission is granted, will the page action stay visible?
cool, but you need tests. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:100: return; much as it's tempting to early-return here I think it's a little confusing given you conditionally execute code further up in the function. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:136: const Extension* extension = ExtensionRegistry::Get(profile) all call sites actually have access to an Extension so just pass that in rather than |extension_id|. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:169: scoped_ptr<ExtensionAction> action(GetBrowserAction(extension)); I don't think this is right? the browser actions are owned by this class, and if you pass ownership out it'll break. I think given the comment below you'll want to copy these so that you can modify the action type. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:179: new ExtensionAction(extension.id(), ActionInfo::TYPE_PAGE, ActionInfo())); TYPE_PAGE isn't necessarily right. you'll need to pass the page type into this method. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.h (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.h:45: // Excludes system indicators. eh, I'd remove that second sentence, implementation detail.
https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:100: return; On 2014/08/01 18:43:37, kalman wrote: > much as it's tempting to early-return here I think it's a little confusing given > you conditionally execute code further up in the function. Hmm. How would you prefer this to be organized? The goal here is to check the title and update it if necessary, then if we find an icon that we can use to replace any missing icons from the action's key, use it. But if not, we shouldn't make any attempts to change icon stuff. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:136: const Extension* extension = ExtensionRegistry::Get(profile) On 2014/08/01 18:43:37, kalman wrote: > all call sites actually have access to an Extension so just pass that in rather > than |extension_id|. I'll pass in a const Extension* since we have a null check on extension below. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:169: scoped_ptr<ExtensionAction> action(GetBrowserAction(extension)); On 2014/08/01 18:43:37, kalman wrote: > I don't think this is right? the browser actions are owned by this class, and if > you pass ownership out it'll break. > > I think given the comment below you'll want to copy these so that you can modify > the action type. Ahh, okay, so this method shouldn't return a reference to this action, it should return an entirely new action that can be manipulated and used? I think that's where my misunderstanding comes from here. If that's correct, this makes a lot more sense. https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:179: new ExtensionAction(extension.id(), ActionInfo::TYPE_PAGE, ActionInfo())); On 2014/08/01 18:43:37, kalman wrote: > TYPE_PAGE isn't necessarily right. you'll need to pass the page type into this > method. So we should pass in the type of action we'd like to get back? If we find a browser action, we should make a copy of it and change its type to page if TYPE_PAGE is passed in? https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.h (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.h:45: // Excludes system indicators. On 2014/08/01 18:43:37, kalman wrote: > eh, I'd remove that second sentence, implementation detail. Fair enough.
Where should I be adding tests for this? The ExtensionActionManager doesn't have a corresponding unittest, but since the whole point of this has to do with action icons, should I add tests to extension_action_icon_factory_unittest.cc? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
you should add a new test specifically for extension action manager.
Here's some tests. What do you think?
nice! https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:179: new ExtensionAction(extension.id(), ActionInfo::TYPE_PAGE, ActionInfo())); On 2014/08/01 20:23:10, gpdavis wrote: > On 2014/08/01 18:43:37, kalman wrote: > > TYPE_PAGE isn't necessarily right. you'll need to pass the page type into this > > method. > > So we should pass in the type of action we'd like to get back? If we find a > browser action, we should make a copy of it and change its type to page if > TYPE_PAGE is passed in? yeah exactly. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:99: if (icon_path.empty()) ... if (!icon_path.empty()) { ... } } is how I'd like you to arrange it. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:54: else why this? seems odd, and you never pass in NULL anyway. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:78: InitializeEmptyExtensionService(); ok let's fix this. there's no good reason for ExtensionActionManager to depend on an ExtensionService, it can just depend on ExtensionRegistry. replace this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... with ExtensionRegistry::Get(profile)->enabled_extensions().Contains(extension_id); then this test can extend ExtensionsTest not ExtensionServiceTestBase. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); and it would be nice to test both page and browser actions here. an idea: 1. Pull this whole test into a function which takes a Callback. 2. This Callback takes an Extension then returns an ExtensionAction. 3. Run that Callback instead of calling manager->GetPageAction. it would look something like this: void TestPopulateMissingValues( const base::Callback<const ExtensionAction*(const Extension*)>& getter) { ... the stuff you already have, then... const ExtensionAction* action = getter.Run(extension.get()); ... etc } then you have TEST_F(ExtensionActionManagerTest, PopulatePageAction) { TestPopulateMissingValues(base::Bind( &ExtensionActionManager::GetPageAction, base::Unretained(manager()))); } similar for PopulateBrowserAction. you should add a manager() or action_manager or whatever method as a shortcut.
https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_action_manager.cc:179: new ExtensionAction(extension.id(), ActionInfo::TYPE_PAGE, ActionInfo())); On 2014/08/05 00:40:09, kalman wrote: > On 2014/08/01 20:23:10, gpdavis wrote: > > On 2014/08/01 18:43:37, kalman wrote: > > > TYPE_PAGE isn't necessarily right. you'll need to pass the page type into > this > > > method. > > > > So we should pass in the type of action we'd like to get back? If we find a > > browser action, we should make a copy of it and change its type to page if > > TYPE_PAGE is passed in? > > yeah exactly. Solid. Patch set 6 has this change. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:99: if (icon_path.empty()) On 2014/08/05 00:40:09, kalman wrote: > ... > if (!icon_path.empty()) { > ... > } > } > > > is how I'd like you to arrange it. Done (will show up in next patch set). https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:54: else On 2014/08/05 00:40:09, kalman wrote: > why this? seems odd, and you never pass in NULL anyway. Well, I figured I'd add a default name because it's the only one of the four keys passed in that's mandatory in the manifest. But you're right; I always pass it in, so it's not really necessary. I just wanted it to be valid if I passed all NULL values into this method. https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:78: InitializeEmptyExtensionService(); On 2014/08/05 00:40:09, kalman wrote: > ok let's fix this. there's no good reason for ExtensionActionManager to depend > on an ExtensionService, it can just depend on ExtensionRegistry. replace this: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > with > ExtensionRegistry::Get(profile)->enabled_extensions().Contains(extension_id); > > then this test can extend ExtensionsTest not ExtensionServiceTestBase. I already changed the code you're referring to, since we wanted to pass in an extension instead of an ID. I have it passing in an Extension*, and I just NULL check it (this is okay, right?) Can this test be an ExtensionsTest? The header says to only use this class in extensions_unittests: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... Also, I need a profile to get the ExtensionActionManager, and ExtensionsTest provides a browser_context. I got a segfault when I tried to use Profile::FromBrowserContext(). I also had to case the TestBrowserContext to a BrowserContext, and it seemed like a recipe for disaster anyway. Advice? https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); On 2014/08/05 00:40:09, kalman wrote: > and it would be nice to test both page and browser actions here. an idea: > > 1. Pull this whole test into a function which takes a Callback. > 2. This Callback takes an Extension then returns an ExtensionAction. > 3. Run that Callback instead of calling manager->GetPageAction. > > it would look something like this: > > void TestPopulateMissingValues( > const base::Callback<const ExtensionAction*(const Extension*)>& getter) { > ... the stuff you already have, then... > const ExtensionAction* action = getter.Run(extension.get()); > ... etc > } > > then you have > > TEST_F(ExtensionActionManagerTest, PopulatePageAction) { > TestPopulateMissingValues(base::Bind( > &ExtensionActionManager::GetPageAction, > base::Unretained(manager()))); > } > > similar for PopulateBrowserAction. > > you should add a manager() or action_manager or whatever method as a shortcut. I'm having trouble getting the callback to work. I think it has to do with reference counting: https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73 ExtensionActionManager doesn't appear to support RefCounted, and I'm getting a compiler error. Is there a way around this, or am I misunderstanding something?
https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); On 2014/08/05 18:44:15, gpdavis wrote: > On 2014/08/05 00:40:09, kalman wrote: > > and it would be nice to test both page and browser actions here. an idea: > > > > 1. Pull this whole test into a function which takes a Callback. > > 2. This Callback takes an Extension then returns an ExtensionAction. > > 3. Run that Callback instead of calling manager->GetPageAction. > > > > it would look something like this: > > > > void TestPopulateMissingValues( > > const base::Callback<const ExtensionAction*(const Extension*)>& getter) { > > ... the stuff you already have, then... > > const ExtensionAction* action = getter.Run(extension.get()); > > ... etc > > } > > > > then you have > > > > TEST_F(ExtensionActionManagerTest, PopulatePageAction) { > > TestPopulateMissingValues(base::Bind( > > &ExtensionActionManager::GetPageAction, > > base::Unretained(manager()))); > > } > > > > similar for PopulateBrowserAction. > > > > you should add a manager() or action_manager or whatever method as a shortcut. > > I'm having trouble getting the callback to work. I think it has to do with > reference counting: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73 > > ExtensionActionManager doesn't appear to support RefCounted, and I'm getting a > compiler error. Is there a way around this, or am I misunderstanding something? are you using base::Unretained()?
https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); On 2014/08/05 18:48:53, kalman wrote: > On 2014/08/05 18:44:15, gpdavis wrote: > > On 2014/08/05 00:40:09, kalman wrote: > > > and it would be nice to test both page and browser actions here. an idea: > > > > > > 1. Pull this whole test into a function which takes a Callback. > > > 2. This Callback takes an Extension then returns an ExtensionAction. > > > 3. Run that Callback instead of calling manager->GetPageAction. > > > > > > it would look something like this: > > > > > > void TestPopulateMissingValues( > > > const base::Callback<const ExtensionAction*(const Extension*)>& getter) > { > > > ... the stuff you already have, then... > > > const ExtensionAction* action = getter.Run(extension.get()); > > > ... etc > > > } > > > > > > then you have > > > > > > TEST_F(ExtensionActionManagerTest, PopulatePageAction) { > > > TestPopulateMissingValues(base::Bind( > > > &ExtensionActionManager::GetPageAction, > > > base::Unretained(manager()))); > > > } > > > > > > similar for PopulateBrowserAction. > > > > > > you should add a manager() or action_manager or whatever method as a > shortcut. > > > > I'm having trouble getting the callback to work. I think it has to do with > > reference counting: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73 > > > > ExtensionActionManager doesn't appear to support RefCounted, and I'm getting a > > compiler error. Is there a way around this, or am I misunderstanding > something? > > are you using base::Unretained()? Yep. I was doing it just as you said, except that GetPageAction takes a const Extension&, not an Extension*, so I adjusted the callback accordingly.
https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); On 2014/08/05 18:50:57, gpdavis wrote: > On 2014/08/05 18:48:53, kalman wrote: > > On 2014/08/05 18:44:15, gpdavis wrote: > > > On 2014/08/05 00:40:09, kalman wrote: > > > > and it would be nice to test both page and browser actions here. an idea: > > > > > > > > 1. Pull this whole test into a function which takes a Callback. > > > > 2. This Callback takes an Extension then returns an ExtensionAction. > > > > 3. Run that Callback instead of calling manager->GetPageAction. > > > > > > > > it would look something like this: > > > > > > > > void TestPopulateMissingValues( > > > > const base::Callback<const ExtensionAction*(const Extension*)>& > getter) > > { > > > > ... the stuff you already have, then... > > > > const ExtensionAction* action = getter.Run(extension.get()); > > > > ... etc > > > > } > > > > > > > > then you have > > > > > > > > TEST_F(ExtensionActionManagerTest, PopulatePageAction) { > > > > TestPopulateMissingValues(base::Bind( > > > > &ExtensionActionManager::GetPageAction, > > > > base::Unretained(manager()))); > > > > } > > > > > > > > similar for PopulateBrowserAction. > > > > > > > > you should add a manager() or action_manager or whatever method as a > > shortcut. > > > > > > I'm having trouble getting the callback to work. I think it has to do with > > > reference counting: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73 > > > > > > ExtensionActionManager doesn't appear to support RefCounted, and I'm getting > a > > > compiler error. Is there a way around this, or am I misunderstanding > > something? > > > > are you using base::Unretained()? > > Yep. I was doing it just as you said, except that GetPageAction takes a const > Extension&, not an Extension*, so I adjusted the callback accordingly. Ah, I got it. GetPageAction doesn't return a const pointer. That was the problem, and the cryptic nature of the compiler errors with these callbacks made it difficult to determine that that was the problem. Still need advice on switching to ExtensionsTest. Can't those only be run from the extensions/ directory?
https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:90: const ExtensionAction* action = manager->GetPageAction(*extension); On 2014/08/05 19:07:59, gpdavis wrote: > On 2014/08/05 18:50:57, gpdavis wrote: > > On 2014/08/05 18:48:53, kalman wrote: > > > On 2014/08/05 18:44:15, gpdavis wrote: > > > > On 2014/08/05 00:40:09, kalman wrote: > > > > > and it would be nice to test both page and browser actions here. an > idea: > > > > > > > > > > 1. Pull this whole test into a function which takes a Callback. > > > > > 2. This Callback takes an Extension then returns an ExtensionAction. > > > > > 3. Run that Callback instead of calling manager->GetPageAction. > > > > > > > > > > it would look something like this: > > > > > > > > > > void TestPopulateMissingValues( > > > > > const base::Callback<const ExtensionAction*(const Extension*)>& > > getter) > > > { > > > > > ... the stuff you already have, then... > > > > > const ExtensionAction* action = getter.Run(extension.get()); > > > > > ... etc > > > > > } > > > > > > > > > > then you have > > > > > > > > > > TEST_F(ExtensionActionManagerTest, PopulatePageAction) { > > > > > TestPopulateMissingValues(base::Bind( > > > > > &ExtensionActionManager::GetPageAction, > > > > > base::Unretained(manager()))); > > > > > } > > > > > > > > > > similar for PopulateBrowserAction. > > > > > > > > > > you should add a manager() or action_manager or whatever method as a > > > shortcut. > > > > > > > > I'm having trouble getting the callback to work. I think it has to do > with > > > > reference counting: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73 > > > > > > > > ExtensionActionManager doesn't appear to support RefCounted, and I'm > getting > > a > > > > compiler error. Is there a way around this, or am I misunderstanding > > > something? > > > > > > are you using base::Unretained()? > > > > Yep. I was doing it just as you said, except that GetPageAction takes a const > > Extension&, not an Extension*, so I adjusted the callback accordingly. > > Ah, I got it. GetPageAction doesn't return a const pointer. That was the > problem, and the cryptic nature of the compiler errors with these callbacks made > it difficult to determine that that was the problem. > > Still need advice on switching to ExtensionsTest. Can't those only be run from > the extensions/ directory? Double nevermind. Sorry for the repeated comments. I added a testing profile to this class, and made it a subclass of testing::Test instead of ExtensionsTest (hope that's okay). Everything seems to be functioning properly now. Have a look and let me know what you think.
looking good, just test comments left. https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:18: ExtensionActionManagerTest(); everything from here down can be protected: I think. https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:91: somewhere in here we should also test having multiple icons, and only icons that are smaller, and both. that way we won't regress that bug filed ages ago about low-res icons. https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:101: ASSERT_TRUE(extension.get()); it's better if you make this assertion inside the BuildExtension method so that you don't need to keep doing ASSERT_TRUE(extension.get()) https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:161: assert that it matches the page action? https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:174: ASSERT_TRUE(IconsMatch(*extension, *action)); assert that it matches the page action?
https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:18: ExtensionActionManagerTest(); On 2014/08/05 21:20:41, kalman wrote: > everything from here down can be protected: I think. Done. https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:91: On 2014/08/05 21:20:40, kalman wrote: > somewhere in here we should also test having multiple icons, and only icons that > are smaller, and both. that way we won't regress that bug filed ages ago about > low-res icons. Currently, I have it grabbing the largest icon it can find in the "icons" key, and using that to replace any keys in the action's "default_icon" that are missing. The only instance in which I can imagine this being a problem is here: "icons": { "16": "icon16.png" }, "page_action": { "default_icon": { "19": "icon19.png" } } In this case, the 38px icon for the page_action would be populated with icon16.png, when it would be better to use icon19.png. Should we worry much about that? What cases are important to test here? Something like the above situation? https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:101: ASSERT_TRUE(extension.get()); On 2014/08/05 21:20:40, kalman wrote: > it's better if you make this assertion inside the BuildExtension method so that > you don't need to keep doing ASSERT_TRUE(extension.get()) Done. https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:161: On 2014/08/05 21:20:41, kalman wrote: > assert that it matches the page action? Is this a documentation suggestion, or are you suggesting that I use GetPageAction to pull the page action out and compare it to |action|?
https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:91: On 2014/08/05 21:41:46, gpdavis wrote: > On 2014/08/05 21:20:40, kalman wrote: > > somewhere in here we should also test having multiple icons, and only icons > that > > are smaller, and both. that way we won't regress that bug filed ages ago about > > low-res icons. > > Currently, I have it grabbing the largest icon it can find in the "icons" key, > and using that to replace any keys in the action's "default_icon" that are > missing. Yep, and we should test that it does pick the largest. > The only instance in which I can imagine this being a problem is here: > > "icons": { > "16": "icon16.png" > }, > "page_action": { > "default_icon": { > "19": "icon19.png" > } > } > > In this case, the 38px icon for the page_action would be populated with > icon16.png, when it would be better to use icon19.png. Should we worry much > about that? > > What cases are important to test here? Something like the above situation? yep that's exactly what we should test - the default icon of the page action shouldn't be overridden by a smaller icon. however, even if that default icon wasn't specified it *still* shouldn't be overridden by that 16px icon, because that's smaller than the page action size.
https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:91: On 2014/08/05 21:45:31, kalman wrote: > On 2014/08/05 21:41:46, gpdavis wrote: > > On 2014/08/05 21:20:40, kalman wrote: > > > somewhere in here we should also test having multiple icons, and only icons > > that > > > are smaller, and both. that way we won't regress that bug filed ages ago > about > > > low-res icons. > > > > Currently, I have it grabbing the largest icon it can find in the "icons" key, > > and using that to replace any keys in the action's "default_icon" that are > > missing. > > Yep, and we should test that it does pick the largest. > > > The only instance in which I can imagine this being a problem is here: > > > > "icons": { > > "16": "icon16.png" > > }, > > "page_action": { > > "default_icon": { > > "19": "icon19.png" > > } > > } > > > > In this case, the 38px icon for the page_action would be populated with > > icon16.png, when it would be better to use icon19.png. Should we worry much > > about that? > > > > What cases are important to test here? Something like the above situation? > > yep that's exactly what we should test - the default icon of the page action > shouldn't be overridden by a smaller icon. > > however, even if that default icon wasn't specified it *still* shouldn't be > overridden by that 16px icon, because that's smaller than the page action size. So we prefer the default puzzle piece icon to a 16px icon resized up? https://codereview.chromium.org/415813003/diff/190001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:101: ASSERT_TRUE(extension.get()); On 2014/08/05 21:41:46, gpdavis wrote: > On 2014/08/05 21:20:40, kalman wrote: > > it's better if you make this assertion inside the BuildExtension method so > that > > you don't need to keep doing ASSERT_TRUE(extension.get()) > > Done. Shouldn't have given you a premature done here-- I just didn't anticipate anything going wrong with this. I can't do this check within the method because it returns a scoped_refptr<Extension>, and the macro can return. I get a compiler error about how there's "no viable conversion from void to scoped_refptr<extensions::Extension>". Is there a way around this?
Here are some more thorough tests.
https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:86: const int* kIconSizes = extension_misc::kExtensionActionIconSizes; should this be const int kIconSizes[] = ...? or does the compiler not like that. I can never remember. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:87: const int kNumIcons = extension_misc::kNumExtensionActionIconSizes; not much point making these scoped outside of the only function which uses them. (and this should really be kNumIconSizes) (and it should be a size_t) https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:102: // for the 19px key and return; why this? seems very specific to 19 vs 38 px icons. I would say that a more general approach is: (I think, if that's what you're going for): for (size_t i = 0; i < kNumIconSizes; ++i) { int i_icon_size = kIconSizes[i]; if (!default_icon->Get(i_icon_size).empty()) continue; // Find the smallest icon which can be cleanly scaled down to this one. for (size_t j = i + 1; j < kNumIconSizes; ++j) { int j_icon_size = jIconSizes[j]; if (j_icon_size % i_icon_size != 0) continue; const std::string& path = default_icon->Get(j_icon_size); if (path.empty()) continue; default_icon->Add( i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY); break; } } } but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you automatically? maybe I'm misunderstanding. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:125: default_icon->Add(size, largest_icon); {} around this. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:99: (action_type == kBrowserAction) ? why not just pass ExtensionActionManager::Get{Browser,Page}Action into this method directly? https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:110: extension_icons->Pass(), there are a lot of instances in this test where you can use the builder pattern more effectively. e.g. here, you can do scoped_refptr<Extension> extension = BuildExtension( DictionaryBuilder().Set("48", "icon48.png") .Set("128", "icon128.png"))); or whatever makes that compile. same elsewhere. basically the point of DictionaryBuilder is to avoid local variables as much as possible. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:210: ASSERT_FALSE(TitlesMatch(*extension, *action)); it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) I think. as in, sure, this assertion is good, but this would also pass if the title is empty. better to assert exactly what it should be.
https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:86: const int* kIconSizes = extension_misc::kExtensionActionIconSizes; On 2014/08/07 16:03:37, kalman wrote: > should this be const int kIconSizes[] = ...? or does the compiler not like that. > I can never remember. Well, I was trying to avoid copying the array by just creating a pointer to the start of the array. Either way would work, and if the array implementation is preferred, I could do that. I just figured this was ever so marginally better because it functions as an array anyway *shrug* https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:87: const int kNumIcons = extension_misc::kNumExtensionActionIconSizes; On 2014/08/07 16:03:37, kalman wrote: > not much point making these scoped outside of the only function which uses them. > > (and this should really be kNumIconSizes) > (and it should be a size_t) Done. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:102: // for the 19px key and return; On 2014/08/07 16:03:37, kalman wrote: > why this? seems very specific to 19 vs 38 px icons. > > I would say that a more general approach is: (I think, if that's what you're > going for): > > for (size_t i = 0; i < kNumIconSizes; ++i) { > int i_icon_size = kIconSizes[i]; > if (!default_icon->Get(i_icon_size).empty()) > continue; > // Find the smallest icon which can be cleanly scaled down to this one. > for (size_t j = i + 1; j < kNumIconSizes; ++j) { > int j_icon_size = jIconSizes[j]; > if (j_icon_size % i_icon_size != 0) > continue; > const std::string& path = default_icon->Get(j_icon_size); > if (path.empty()) > continue; > default_icon->Add( > i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY); > break; > } > } > } > > but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you > automatically? maybe I'm misunderstanding. It certainly is specific to 19 and 38px icons. Essentially the goal here was to condense a rather convoluted set of logic as much as possible. Perhaps a discussion on how exactly we want to approach these situations is worthwhile. What I've implemented (and correct me if any of these assumptions are incorrect): 1. We don't want to replace missing action icons with extension icons if there is another viable action icon to use instead. The only time this manifests itself currently is when there's a 38px icon but no 19px icon; in this case, we scale down the 38, and we're done because 19 and 38 have icons. 2. I was under the impression that we want to use the largest extension icon for substitution if case 1) doesn't happen. But what if the extension's largest icon is 24px? We could use that for 19 (right?), but we couldn't use it for 38. This is what the last for loop is for. So my approach was: Check condition 1) by seeing if 38 exists but 19 doesn't, in which case we set 19 to 38's icon and return (since all action icons are now filled). Otherwise, we retrieve the largest extension icon we can find, and if we find one, we go through the action icons keys, replacing anything that satisfies the conditions: the icon must be missing from the action key, and the resolution of the key must be smaller than the largest extension icon we have. This could certainly be restructured as a loop that goes through each action icon and makes the determination on what to do. But I noticed that modulo operation, and I'm wondering if there's more to scaling icons than I thought. I was under the impression that any -larger- icon is a viable option, but it looks like you're suggesting that the resolution ought to be a multiple. So how about this. Loop through each action icon. If the icon is missing, first search any remaining larger action icons for one that has a resolution that modulos cleanly (which covers the case I described above, since the action icons are 19 and 38). If we can't find one, then we consult the extension icons in the same way: find a larger icon that has a resolution that divides cleanly. I could use the array of extension icon sizes for this. My only concern is that the sets of icon sizes don't match up; under these conditions, the only extension icon that will be chosen for 19px would be the 19px icon, since none of the other resolutions modulo with 19 cleanly. Sorry for the long-winded response. This should be easy implementation if I get solidify exactly what we ought to be doing here. What should the rules be for resizing icons? https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:125: default_icon->Add(size, largest_icon); On 2014/08/07 16:03:37, kalman wrote: > {} around this. Done. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:99: (action_type == kBrowserAction) ? On 2014/08/07 16:03:37, kalman wrote: > why not just pass ExtensionActionManager::Get{Browser,Page}Action into this > method directly? Well, I figured since we already need to pass |action_type| in for setting the extension manifests, there's no need to have to pass in the callback, since it can be determined by |action_type|. No good? https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:110: extension_icons->Pass(), On 2014/08/07 16:03:37, kalman wrote: > there are a lot of instances in this test where you can use the builder pattern > more effectively. e.g. here, you can do > > scoped_refptr<Extension> extension = BuildExtension( > DictionaryBuilder().Set("48", "icon48.png") > .Set("128", "icon128.png"))); > > or whatever makes that compile. > > same elsewhere. basically the point of DictionaryBuilder is to avoid local > variables as much as possible. I was primarily concerned with the readability and formatting of this code, since nesting DictionaryBuilder constructors can be really messy to format. I'll do it that way if you'd prefer, but I like the structure of this function because it follows a simple pattern: for each test, build the extension_icons dictionary, build the action dictionary (if applicable), build the extension, get the action, and make assertions. I figure that's pretty simple to understand, but you're right, having to reset those pointers all over the place isn't exactly necessary. Your call :) https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:210: ASSERT_FALSE(TitlesMatch(*extension, *action)); On 2014/08/07 16:03:37, kalman wrote: > it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) I > think. as in, sure, this assertion is good, but this would also pass if the > title is empty. better to assert exactly what it should be. I'm a little confused-- why would this pass if the title is empty? It should only pass if both titles are empty.
https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:86: const int* kIconSizes = extension_misc::kExtensionActionIconSizes; On 2014/08/07 18:30:59, gpdavis wrote: > On 2014/08/07 16:03:37, kalman wrote: > > should this be const int kIconSizes[] = ...? or does the compiler not like > that. > > I can never remember. > > Well, I was trying to avoid copying the array by just creating a pointer to the > start of the array. Either way would work, and if the array implementation is > preferred, I could do that. I just figured this was ever so marginally better > because it functions as an array anyway *shrug* I don't think using [] copies the array. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:102: // for the 19px key and return; On 2014/08/07 18:30:59, gpdavis wrote: > On 2014/08/07 16:03:37, kalman wrote: > > why this? seems very specific to 19 vs 38 px icons. > > > > I would say that a more general approach is: (I think, if that's what you're > > going for): > > > > for (size_t i = 0; i < kNumIconSizes; ++i) { > > int i_icon_size = kIconSizes[i]; > > if (!default_icon->Get(i_icon_size).empty()) > > continue; > > // Find the smallest icon which can be cleanly scaled down to this one. > > for (size_t j = i + 1; j < kNumIconSizes; ++j) { > > int j_icon_size = jIconSizes[j]; > > if (j_icon_size % i_icon_size != 0) > > continue; > > const std::string& path = default_icon->Get(j_icon_size); > > if (path.empty()) > > continue; > > default_icon->Add( > > i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY); > > break; > > } > > } > > } > > > > but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you > > automatically? maybe I'm misunderstanding. > > It certainly is specific to 19 and 38px icons. > > Essentially the goal here was to condense a rather convoluted set of logic as > much as possible. Perhaps a discussion on how exactly we want to approach these > situations is worthwhile. What I've implemented (and correct me if any of these > assumptions are incorrect): > > 1. We don't want to replace missing action icons with extension icons if there > is another viable action icon to use instead. The only time this manifests > itself currently is when there's a 38px icon but no 19px icon; in this case, we > scale down the 38, and we're done because 19 and 38 have icons. > > 2. I was under the impression that we want to use the largest extension icon for > substitution if case 1) doesn't happen. But what if the extension's largest > icon is 24px? We could use that for 19 (right?), but we couldn't use it for 38. > This is what the last for loop is for. > > > So my approach was: > > Check condition 1) by seeing if 38 exists but 19 doesn't, in which case we set > 19 to 38's icon and return (since all action icons are now filled). > > Otherwise, we retrieve the largest extension icon we can find, and if we find > one, we go through the action icons keys, replacing anything that satisfies the > conditions: the icon must be missing from the action key, and the resolution of > the key must be smaller than the largest extension icon we have. > > > This could certainly be restructured as a loop that goes through each action > icon and makes the determination on what to do. But I noticed that modulo > operation, and I'm wondering if there's more to scaling icons than I thought. I > was under the impression that any -larger- icon is a viable option, but it looks > like you're suggesting that the resolution ought to be a multiple. > > So how about this. Loop through each action icon. If the icon is missing, > first search any remaining larger action icons for one that has a resolution > that modulos cleanly (which covers the case I described above, since the action > icons are 19 and 38). If we can't find one, then we consult the extension icons > in the same way: find a larger icon that has a resolution that divides cleanly. > I could use the array of extension icon sizes for this. My only concern is that > the sets of icon sizes don't match up; under these conditions, the only > extension icon that will be chosen for 19px would be the 19px icon, since none > of the other resolutions modulo with 19 cleanly. > > Sorry for the long-winded response. This should be easy implementation if I get > solidify exactly what we ought to be doing here. What should the rules be for > resizing icons? Yes the modulo thing was so that we get cleaner icon scaling, but some kind of icon is better than no icon (provided the resolution isn't crappy). but really it seems like based on kExtensionIconSizes vs kExtensionActionIconSizes there is actually no clean conversion. the 38 --> 19 conversion should already be happening with that Get() method, right? so what would be wrong with a super simple algorithm, find the largest extension icon, and if it's larger than the required extension action size, assign it?
forgot to respond to your other questions. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:99: (action_type == kBrowserAction) ? On 2014/08/07 18:31:00, gpdavis wrote: > On 2014/08/07 16:03:37, kalman wrote: > > why not just pass ExtensionActionManager::Get{Browser,Page}Action into this > > method directly? > > Well, I figured since we already need to pass |action_type| in for setting the > extension manifests, there's no need to have to pass in the callback, since it > can be determined by |action_type|. No good? ah, fair enough. I didn't notice you needed to pass the action type in to construct these. in that case using a callback is a rather roundabout way to do it. perhaps you can just define a method like GetAction(action_type) and use that rather than getter.Run(). https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:110: extension_icons->Pass(), On 2014/08/07 18:31:00, gpdavis wrote: > On 2014/08/07 16:03:37, kalman wrote: > > there are a lot of instances in this test where you can use the builder > pattern > > more effectively. e.g. here, you can do > > > > scoped_refptr<Extension> extension = BuildExtension( > > DictionaryBuilder().Set("48", "icon48.png") > > .Set("128", "icon128.png"))); > > > > or whatever makes that compile. > > > > same elsewhere. basically the point of DictionaryBuilder is to avoid local > > variables as much as possible. > > I was primarily concerned with the readability and formatting of this code, > since nesting DictionaryBuilder constructors can be really messy to format. > I'll do it that way if you'd prefer, but I like the structure of this function > because it follows a simple pattern: for each test, build the extension_icons > dictionary, build the action dictionary (if applicable), build the extension, > get the action, and make assertions. I figure that's pretty simple to > understand, but you're right, having to reset those pointers all over the place > isn't exactly necessary. > > Your call :) yeah it looks odd to me, for example |extension_icons| being reset every time with a dictionary builder. most of the time these builders are supposed to be temporary objects, you might as well just construct using a plain DictionaryValue instead. anyway, I do prefer using as few temporary variables as possible. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:210: ASSERT_FALSE(TitlesMatch(*extension, *action)); On 2014/08/07 18:31:00, gpdavis wrote: > On 2014/08/07 16:03:37, kalman wrote: > > it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) I > > think. as in, sure, this assertion is good, but this would also pass if the > > title is empty. better to assert exactly what it should be. > > I'm a little confused-- why would this pass if the title is empty? It should > only pass if both titles are empty. TitlesMatch("Test Extension", "") --> false --> ASSERT_FALSE passes?
https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:86: const int* kIconSizes = extension_misc::kExtensionActionIconSizes; On 2014/08/07 19:03:28, kalman wrote: > On 2014/08/07 18:30:59, gpdavis wrote: > > On 2014/08/07 16:03:37, kalman wrote: > > > should this be const int kIconSizes[] = ...? or does the compiler not like > > that. > > > I can never remember. > > > > Well, I was trying to avoid copying the array by just creating a pointer to > the > > start of the array. Either way would work, and if the array implementation is > > preferred, I could do that. I just figured this was ever so marginally better > > because it functions as an array anyway *shrug* > > I don't think using [] copies the array. You were right about the compiler not liking it: const int kIconSizes[] = extension_misc::kExtensionActionIconSizes; "Array initializer must be an initializer list" I thought perhaps that would copy the array contents over. In any case, I think the pointer is what we're looking for. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:102: // for the 19px key and return; On 2014/08/07 19:03:28, kalman wrote: > On 2014/08/07 18:30:59, gpdavis wrote: > > On 2014/08/07 16:03:37, kalman wrote: > > > why this? seems very specific to 19 vs 38 px icons. > > > > > > I would say that a more general approach is: (I think, if that's what you're > > > going for): > > > > > > for (size_t i = 0; i < kNumIconSizes; ++i) { > > > int i_icon_size = kIconSizes[i]; > > > if (!default_icon->Get(i_icon_size).empty()) > > > continue; > > > // Find the smallest icon which can be cleanly scaled down to this one. > > > for (size_t j = i + 1; j < kNumIconSizes; ++j) { > > > int j_icon_size = jIconSizes[j]; > > > if (j_icon_size % i_icon_size != 0) > > > continue; > > > const std::string& path = default_icon->Get(j_icon_size); > > > if (path.empty()) > > > continue; > > > default_icon->Add( > > > i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY); > > > break; > > > } > > > } > > > } > > > > > > but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you > > > automatically? maybe I'm misunderstanding. > > > > It certainly is specific to 19 and 38px icons. > > > > Essentially the goal here was to condense a rather convoluted set of logic as > > much as possible. Perhaps a discussion on how exactly we want to approach > these > > situations is worthwhile. What I've implemented (and correct me if any of > these > > assumptions are incorrect): > > > > 1. We don't want to replace missing action icons with extension icons if there > > is another viable action icon to use instead. The only time this manifests > > itself currently is when there's a 38px icon but no 19px icon; in this case, > we > > scale down the 38, and we're done because 19 and 38 have icons. > > > > 2. I was under the impression that we want to use the largest extension icon > for > > substitution if case 1) doesn't happen. But what if the extension's largest > > icon is 24px? We could use that for 19 (right?), but we couldn't use it for > 38. > > This is what the last for loop is for. > > > > > > So my approach was: > > > > Check condition 1) by seeing if 38 exists but 19 doesn't, in which case we set > > 19 to 38's icon and return (since all action icons are now filled). > > > > Otherwise, we retrieve the largest extension icon we can find, and if we find > > one, we go through the action icons keys, replacing anything that satisfies > the > > conditions: the icon must be missing from the action key, and the resolution > of > > the key must be smaller than the largest extension icon we have. > > > > > > This could certainly be restructured as a loop that goes through each action > > icon and makes the determination on what to do. But I noticed that modulo > > operation, and I'm wondering if there's more to scaling icons than I thought. > I > > was under the impression that any -larger- icon is a viable option, but it > looks > > like you're suggesting that the resolution ought to be a multiple. > > > > So how about this. Loop through each action icon. If the icon is missing, > > first search any remaining larger action icons for one that has a resolution > > that modulos cleanly (which covers the case I described above, since the > action > > icons are 19 and 38). If we can't find one, then we consult the extension > icons > > in the same way: find a larger icon that has a resolution that divides > cleanly. > > I could use the array of extension icon sizes for this. My only concern is > that > > the sets of icon sizes don't match up; under these conditions, the only > > extension icon that will be chosen for 19px would be the 19px icon, since none > > of the other resolutions modulo with 19 cleanly. > > > > Sorry for the long-winded response. This should be easy implementation if I > get > > solidify exactly what we ought to be doing here. What should the rules be for > > resizing icons? > > Yes the modulo thing was so that we get cleaner icon scaling, but some kind of > icon is better than no icon (provided the resolution isn't crappy). but really > it seems like based on kExtensionIconSizes vs kExtensionActionIconSizes there is > actually no clean conversion. > > the 38 --> 19 conversion should already be happening with that Get() method, > right? so what would be wrong with a super simple algorithm, find the largest > extension icon, and if it's larger than the required extension action size, > assign it? This is essentially what I have, except I'm passing in MATCH_EXACTLY, so that conversion does not happen in Get(). But the 38px icon is already pulled for page and browser actions if the 19px icon doesn't exist. So then in that case, we don't really need to replace the 19px icon at all, right? I think I understand what you're saying now. Loop through all action icons, getting with MATCH_BIGGER, and anything that doesn't exist should get replaced with the largest extension icon we can find, so long as it's larger than the action icon we're considering. Sounds reasonable to me. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:99: (action_type == kBrowserAction) ? On 2014/08/07 20:50:18, kalman wrote: > On 2014/08/07 18:31:00, gpdavis wrote: > > On 2014/08/07 16:03:37, kalman wrote: > > > why not just pass ExtensionActionManager::Get{Browser,Page}Action into this > > > method directly? > > > > Well, I figured since we already need to pass |action_type| in for setting the > > extension manifests, there's no need to have to pass in the callback, since it > > can be determined by |action_type|. No good? > > ah, fair enough. I didn't notice you needed to pass the action type in to > construct these. > > in that case using a callback is a rather roundabout way to do it. perhaps you > can just define a method like GetAction(action_type) and use that rather than > getter.Run(). Sure, I can dig this idea. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:110: extension_icons->Pass(), On 2014/08/07 20:50:18, kalman wrote: > On 2014/08/07 18:31:00, gpdavis wrote: > > On 2014/08/07 16:03:37, kalman wrote: > > > there are a lot of instances in this test where you can use the builder > > pattern > > > more effectively. e.g. here, you can do > > > > > > scoped_refptr<Extension> extension = BuildExtension( > > > DictionaryBuilder().Set("48", "icon48.png") > > > .Set("128", "icon128.png"))); > > > > > > or whatever makes that compile. > > > > > > same elsewhere. basically the point of DictionaryBuilder is to avoid local > > > variables as much as possible. > > > > I was primarily concerned with the readability and formatting of this code, > > since nesting DictionaryBuilder constructors can be really messy to format. > > I'll do it that way if you'd prefer, but I like the structure of this function > > because it follows a simple pattern: for each test, build the extension_icons > > dictionary, build the action dictionary (if applicable), build the extension, > > get the action, and make assertions. I figure that's pretty simple to > > understand, but you're right, having to reset those pointers all over the > place > > isn't exactly necessary. > > > > Your call :) > > yeah it looks odd to me, for example |extension_icons| being reset every time > with a dictionary builder. most of the time these builders are supposed to be > temporary objects, you might as well just construct using a plain > DictionaryValue instead. > > anyway, I do prefer using as few temporary variables as possible. Alrighty. Will rewrite with inline DictionaryBuilders instead of local variables. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:210: ASSERT_FALSE(TitlesMatch(*extension, *action)); On 2014/08/07 20:50:18, kalman wrote: > On 2014/08/07 18:31:00, gpdavis wrote: > > On 2014/08/07 16:03:37, kalman wrote: > > > it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) I > > > think. as in, sure, this assertion is good, but this would also pass if the > > > title is empty. better to assert exactly what it should be. > > > > I'm a little confused-- why would this pass if the title is empty? It should > > only pass if both titles are empty. > > TitlesMatch("Test Extension", "") --> false --> ASSERT_FALSE passes? Ah, you said ASSERT_TRUE in the first comment. That's why I was confused. ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) would be checking to see that the extension name matches the page action title. Are you suggesting we ought to check equality between the returned browser action's title and the page action's title as per the manifest? Because that would be something more along the lines of... ASSERT_EQ(manager()->GetPageAction(*extension)->GetTitle(ExtensionAction::kDefaultTabId), action->GetTitle(ExtensionAction::kDefaultTabId));
https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:102: // for the 19px key and return; On 2014/08/07 21:28:18, gpdavis wrote: > On 2014/08/07 19:03:28, kalman wrote: > > On 2014/08/07 18:30:59, gpdavis wrote: > > > On 2014/08/07 16:03:37, kalman wrote: > > > > why this? seems very specific to 19 vs 38 px icons. > > > > > > > > I would say that a more general approach is: (I think, if that's what > you're > > > > going for): > > > > > > > > for (size_t i = 0; i < kNumIconSizes; ++i) { > > > > int i_icon_size = kIconSizes[i]; > > > > if (!default_icon->Get(i_icon_size).empty()) > > > > continue; > > > > // Find the smallest icon which can be cleanly scaled down to this one. > > > > for (size_t j = i + 1; j < kNumIconSizes; ++j) { > > > > int j_icon_size = jIconSizes[j]; > > > > if (j_icon_size % i_icon_size != 0) > > > > continue; > > > > const std::string& path = default_icon->Get(j_icon_size); > > > > if (path.empty()) > > > > continue; > > > > default_icon->Add( > > > > i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY); > > > > break; > > > > } > > > > } > > > > } > > > > > > > > but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you > > > > automatically? maybe I'm misunderstanding. > > > > > > It certainly is specific to 19 and 38px icons. > > > > > > Essentially the goal here was to condense a rather convoluted set of logic > as > > > much as possible. Perhaps a discussion on how exactly we want to approach > > these > > > situations is worthwhile. What I've implemented (and correct me if any of > > these > > > assumptions are incorrect): > > > > > > 1. We don't want to replace missing action icons with extension icons if > there > > > is another viable action icon to use instead. The only time this manifests > > > itself currently is when there's a 38px icon but no 19px icon; in this case, > > we > > > scale down the 38, and we're done because 19 and 38 have icons. > > > > > > 2. I was under the impression that we want to use the largest extension icon > > for > > > substitution if case 1) doesn't happen. But what if the extension's largest > > > icon is 24px? We could use that for 19 (right?), but we couldn't use it for > > 38. > > > This is what the last for loop is for. > > > > > > > > > So my approach was: > > > > > > Check condition 1) by seeing if 38 exists but 19 doesn't, in which case we > set > > > 19 to 38's icon and return (since all action icons are now filled). > > > > > > Otherwise, we retrieve the largest extension icon we can find, and if we > find > > > one, we go through the action icons keys, replacing anything that satisfies > > the > > > conditions: the icon must be missing from the action key, and the resolution > > of > > > the key must be smaller than the largest extension icon we have. > > > > > > > > > This could certainly be restructured as a loop that goes through each action > > > icon and makes the determination on what to do. But I noticed that modulo > > > operation, and I'm wondering if there's more to scaling icons than I > thought. > > I > > > was under the impression that any -larger- icon is a viable option, but it > > looks > > > like you're suggesting that the resolution ought to be a multiple. > > > > > > So how about this. Loop through each action icon. If the icon is missing, > > > first search any remaining larger action icons for one that has a resolution > > > that modulos cleanly (which covers the case I described above, since the > > action > > > icons are 19 and 38). If we can't find one, then we consult the extension > > icons > > > in the same way: find a larger icon that has a resolution that divides > > cleanly. > > > I could use the array of extension icon sizes for this. My only concern is > > that > > > the sets of icon sizes don't match up; under these conditions, the only > > > extension icon that will be chosen for 19px would be the 19px icon, since > none > > > of the other resolutions modulo with 19 cleanly. > > > > > > Sorry for the long-winded response. This should be easy implementation if I > > get > > > solidify exactly what we ought to be doing here. What should the rules be > for > > > resizing icons? > > > > Yes the modulo thing was so that we get cleaner icon scaling, but some kind of > > icon is better than no icon (provided the resolution isn't crappy). but really > > it seems like based on kExtensionIconSizes vs kExtensionActionIconSizes there > is > > actually no clean conversion. > > > > the 38 --> 19 conversion should already be happening with that Get() method, > > right? so what would be wrong with a super simple algorithm, find the largest > > extension icon, and if it's larger than the required extension action size, > > assign it? > > This is essentially what I have, except I'm passing in MATCH_EXACTLY, so that > conversion does not happen in Get(). But the 38px icon is already pulled for > page and browser actions if the 19px icon doesn't exist. So then in that case, > we don't really need to replace the 19px icon at all, right? > > I think I understand what you're saying now. Loop through all action icons, > getting with MATCH_BIGGER, and anything that doesn't exist should get replaced > with the largest extension icon we can find, so long as it's larger than the > action icon we're considering. Sounds reasonable to me. Yeah that's what I'm saying. Specifics of 19 and 38 shouldn't come into it, there's already a constant defined somewhere which lists them. https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:210: ASSERT_FALSE(TitlesMatch(*extension, *action)); On 2014/08/07 21:28:18, gpdavis wrote: > On 2014/08/07 20:50:18, kalman wrote: > > On 2014/08/07 18:31:00, gpdavis wrote: > > > On 2014/08/07 16:03:37, kalman wrote: > > > > it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) > I > > > > think. as in, sure, this assertion is good, but this would also pass if > the > > > > title is empty. better to assert exactly what it should be. > > > > > > I'm a little confused-- why would this pass if the title is empty? It > should > > > only pass if both titles are empty. > > > > TitlesMatch("Test Extension", "") --> false --> ASSERT_FALSE passes? > > Ah, you said ASSERT_TRUE in the first comment. That's why I was confused. > ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) would be checking to > see that the extension name matches the page action title. Are you suggesting > we ought to check equality between the returned browser action's title and the > page action's title as per the manifest? Because that would be something more > along the lines of... > > ASSERT_EQ(manager()->GetPageAction(*extension)->GetTitle(ExtensionAction::kDefaultTabId), > action->GetTitle(ExtensionAction::kDefaultTabId)); yep, that's pretty much what I'm suggesting. though the first part of that can be just "Action!" since you're setting it to "Action!" earlier in the method.
Alright, that icon logic does look quite a bit simpler, and the unit tests don't look that bad with inline DictionaryBuilders. Whaddya think?
almost there. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:116: Profile::FromBrowserContext(web_contents()->GetBrowserContext())) you don't need to cast from a Profile, you can directly use web_contentes()->GetBrowserContext() https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:103: std::string largest_icon = extensions::IconsInfo::GetIcons(&extension).Get( instead of extensions::IconsInfo::GetIcons(&extension) use |extension_icons|? https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:106: int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon); move this inside the !largest_icon.empty() check. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:118: } if you step back in this loop rather than forward you can early-exit, because the icons will be in decreasing order of size. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.h (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.h:45: // Gets the best fit ExtensionAction for the given |extension|. you might want to say, "this takes into account the extensions browser or page actions, if any, along with its name and any declared icons". https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:184: extension_icons->Set("48", "icon48.png"); inline extension_icons as well. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:202: "action38.png"); also assert that action is TYPE_BROWSER. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:205: extension_icons.reset(new DictionaryBuilder()); ditto
https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:116: Profile::FromBrowserContext(web_contents()->GetBrowserContext())) On 2014/08/07 22:14:15, kalman wrote: > you don't need to cast from a Profile, you can directly use > web_contentes()->GetBrowserContext() Compiler: "cannot initialize a parameter of type 'Profile *' with an rvalue of type 'content::BrowserContext *'" Isn't Profile a subclass of Browsercontext? So we could pass in a Profile* in place of a BrowserContext*, but not the other way around. Right? https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:103: std::string largest_icon = extensions::IconsInfo::GetIcons(&extension).Get( On 2014/08/07 22:14:15, kalman wrote: > instead of extensions::IconsInfo::GetIcons(&extension) use |extension_icons|? Whoops-- I definitely meant to use that (that's why I cached it in the first place). Done. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:106: int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon); On 2014/08/07 22:14:15, kalman wrote: > move this inside the !largest_icon.empty() check. Done. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:118: } On 2014/08/07 22:14:15, kalman wrote: > if you step back in this loop rather than forward you can early-exit, because > the icons will be in decreasing order of size. Clever. Will adjust unit tests (since previously it would fill them all in if they were all missing, as it started at the bottom). https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.h (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.h:45: // Gets the best fit ExtensionAction for the given |extension|. On 2014/08/07 22:14:15, kalman wrote: > you might want to say, "this takes into account the extensions browser or page > actions, if any, along with its name and any declared icons". Done. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager_unittest.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:184: extension_icons->Set("48", "icon48.png"); On 2014/08/07 22:15:09, kalman wrote: > inline extension_icons as well. Done. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:202: "action38.png"); On 2014/08/07 22:15:10, kalman wrote: > also assert that action is TYPE_BROWSER. Done. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager_unittest.cc:205: extension_icons.reset(new DictionaryBuilder()); On 2014/08/07 22:15:09, kalman wrote: > ditto Done.
ok - lgtm. let's go with this, but there are a couple of other little changes that should be pretty straightforward. https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/415813003/diff/220001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:116: Profile::FromBrowserContext(web_contents()->GetBrowserContext())) On 2014/08/07 23:48:34, gpdavis wrote: > On 2014/08/07 22:14:15, kalman wrote: > > you don't need to cast from a Profile, you can directly use > > web_contentes()->GetBrowserContext() > > Compiler: > "cannot initialize a parameter of type 'Profile *' with an rvalue of type > 'content::BrowserContext *'" > > Isn't Profile a subclass of Browsercontext? So we could pass in a Profile* in > place of a BrowserContext*, but not the other way around. Right? ah, right. I had assumed that ExtensionActionManager used a BrowserContext but it uses a Profile. https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:109: (largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon))) { this looks kind of awkward, can't you declare that largest_icon_size variable inside here? why the need to have the && part of the condition? https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:116: && largest_icon_size > size) { this largest_icon_size>size check is funny - you're stepping down in sizes, so if it's true on the first iteration it'll be true for all future iterations. you might as well include it in the outer for-loop condition. for (int i = kNumIconSizes - 1, size; i >= 0 && (size = kIconSizes[i]) < largest_icon_size; ++i) { ... } something like that should work?
https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:109: (largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon))) { On 2014/08/08 00:21:07, kalman wrote: > this looks kind of awkward, can't you declare that largest_icon_size variable > inside here? why the need to have the && part of the condition? Oh, I misinterpreted what you said. I thought you meant place it inside the conditional statement. Done. https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:116: && largest_icon_size > size) { On 2014/08/08 00:21:07, kalman wrote: > this largest_icon_size>size check is funny - you're stepping down in sizes, so > if it's true on the first iteration it'll be true for all future iterations. you > might as well include it in the outer for-loop condition. > > for (int i = kNumIconSizes - 1, size; > i >= 0 && (size = kIconSizes[i]) < largest_icon_size; > ++i) { > ... > } > > something like that should work? What if the action has no icons, but the extension has a 24px icon? We want the 24 to go into the "19" key, but not the "38" key. An early termination of the loop you're suggesting wouldn't work for that. In fact, I just tweaked a test for this case. This is the behavior we want, right?
lgtm https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:116: && largest_icon_size > size) { On 2014/08/08 01:03:58, gpdavis wrote: > On 2014/08/08 00:21:07, kalman wrote: > > this largest_icon_size>size check is funny - you're stepping down in sizes, so > > if it's true on the first iteration it'll be true for all future iterations. > you > > might as well include it in the outer for-loop condition. > > > > for (int i = kNumIconSizes - 1, size; > > i >= 0 && (size = kIconSizes[i]) < largest_icon_size; > > ++i) { > > ... > > } > > > > something like that should work? > > What if the action has no icons, but the extension has a 24px icon? We want the > 24 to go into the "19" key, but not the "38" key. An early termination of the > loop you're suggesting wouldn't work for that. > > In fact, I just tweaked a test for this case. This is the behavior we want, > right? ah right, of course. now that it's a new day I think I know what I my poor brain was trying to tell me yesterday, and it's what you wrote.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/415813003/2...
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/415813003/2...
The CQ bit was unchecked by gpdavis.chromium@gmail.com
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/415813003/3...
hold up.. https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:144: ->GetExtensionById(extension->id(), ExtensionRegistry::ENABLED)) { why do you need this change?
On 2014/08/08 23:02:13, kalman wrote: > hold up.. > > https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... > File chrome/browser/extensions/extension_action_manager.cc (right): > > https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... > chrome/browser/extensions/extension_action_manager.cc:144: > ->GetExtensionById(extension->id(), ExtensionRegistry::ENABLED)) { > why do you need this change? This test was failing: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I believe this is the case that brought about this check in the first place. It started as a check with ExtensionService, but since that's deprecated, I replaced it with the ExtensionRegistry check.
On 2014/08/08 23:03:42, gpdavis wrote: > On 2014/08/08 23:02:13, kalman wrote: > > hold up.. > > > > > https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... > > File chrome/browser/extensions/extension_action_manager.cc (right): > > > > > https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... > > chrome/browser/extensions/extension_action_manager.cc:144: > > ->GetExtensionById(extension->id(), ExtensionRegistry::ENABLED)) { > > why do you need this change? > > This test was failing: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I believe this is the case that brought about this check in the first place. It > started as a check with ExtensionService, but since that's deprecated, I > replaced it with the ExtensionRegistry check. Here we go: https://chromiumcodereview.appspot.com/20304002
The CQ bit was unchecked by gpdavis.chromium@gmail.com
ok, thanks. please fix that up before submitting. that "it looks like a bug" comment you can do in a follow-up if you feel like it :) (and if it turns out I'm right - it would be easy to test, basically the steps I've said) seriously though, don't make that change in this CL. it's basically read as is. https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:129: const Extension* extension, you should really be passing in a Extension& here to be consistent, with the implication that it can't be null. https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:137: if (!action_info) hmm seems like this "if (!action_info) return NULL" should be the first operation? like, it's a bug that it's not first, and it means that if an extension has a browser action but no page action, you'll see the behaviour: 1- caller gets page action ==> not found in map ==> return NULL 2- caller gets browser action ==> not found in map but does create since there's an action info ==> return the browser action 3- caller gets page action ==> FOUND IN MAP ==> returns the browser action if I'm reading this correctly. maybe there's some deep reason why it's in this order (wish it had a comment though). certainly not anything to do with you :) https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:144: ->GetExtensionById(extension->id(), ExtensionRegistry::ENABLED)) { On 2014/08/08 23:02:13, kalman wrote: > why do you need this change? thanks for explaining this now, it makes sense to me. please use enabled_extensions().Contains though, i.e. ExtensionRegistry::Get(profile)->enabled_extensions().Contains(extension.id())
(lgtm after that)
https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:129: const Extension* extension, On 2014/08/09 03:47:00, kalman wrote: > you should really be passing in a Extension& here to be consistent, with the > implication that it can't be null. Sure thing. https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:137: if (!action_info) On 2014/08/09 03:47:00, kalman wrote: > hmm seems like this "if (!action_info) return NULL" should be the first > operation? like, it's a bug that it's not first, and it means that if an > extension has a browser action but no page action, you'll see the behaviour: > > 1- caller gets page action ==> not found in map ==> return NULL > 2- caller gets browser action ==> not found in map but does create since there's > an action info ==> return the browser action > 3- caller gets page action ==> FOUND IN MAP ==> returns the browser action > > if I'm reading this correctly. maybe there's some deep reason why it's in this > order (wish it had a comment though). certainly not anything to do with you :) Hmm, interesting. Well, like you said, this should be simple to test. I'll investigate in a follow-up CL then. https://codereview.chromium.org/415813003/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_action_manager.cc:144: ->GetExtensionById(extension->id(), ExtensionRegistry::ENABLED)) { On 2014/08/09 03:47:00, kalman wrote: > On 2014/08/08 23:02:13, kalman wrote: > > why do you need this change? > > thanks for explaining this now, it makes sense to me. > > please use enabled_extensions().Contains though, i.e. > ExtensionRegistry::Get(profile)->enabled_extensions().Contains(extension.id()) Sure. Can I ask why this is preferred to GetExtensionById?
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/415813003/3...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/415813003/3...
Message was sent while issue was closed.
Change committed as 289062 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
