Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641413002:
Allow InstallableManager to fetch optional badge icon (Closed)
DescriptionAllow InstallableManager to fetch optional badge icon
This CL updates InstallableManager so that users can specify badge-icon-related
parameters in InstallableParams and retrieve the most suitable badge icon in
InstallableData. Unlike the primary icon, badge icon is optional. Therefore, when a
badge icon is requested yet no suitable icons are found in the web manifest, the
GetData process would still succeed.
The badge icon is to be used for Status Bar Icons for WebAPK's notifications.
BUG=649771
Review-Url: https://codereview.chromium.org/2641413002
Cr-Commit-Position: refs/heads/master@{#447154}
Committed: https://chromium.googlesource.com/chromium/src/+/b205d0d471142ed307b73146e51e3576e4ffcd6e
Patch Set 1 : format #
Total comments: 20
Patch Set 2 : Addressing comments #
Total comments: 6
Patch Set 3 : Addressing comments #Patch Set 4 : Fix rebase #
Messages
Total messages: 40 (30 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Fetch badge icon in InstallableManager BUG= ========== to ========== Allow InstallableManager to fetch optional badge icon This CL updates InstallableManager so that users can specify badge-icon-related parameters in InstallableParams and retrieve the most suitable badge icon in InstallableData. Unlike the primary icon, badge icon is optional. Therefore, when a badge icon is requested yet no suitable icons are found in the web manifest, the GetData process would still succeed. The badge icon is to be used for Status Bar Icons for WebAPK's notifications. BUG=649771 ==========
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/24 19:09:35, F wrote: > Hi Dominick & Peter, PTAL. Thanks! Hi Felix, I'm just thinking about the overall design here. Are we likely to fetch more icon types in the future? Perhaps splashscreen? If that's the case, we might want to actually change the guts of InstallableManager slightly so that at the start, we fill the icons_ vector with everything we want to fetch, then we make a distinct FetchIcons() method called by WorkOnTask() that iterates over icons_, fetching each one in there. That will scale better, and avoid some of the repetitive params construction. Don't make this change yet - let me think about it a bit more. But knowing the answer to that question will help.
On 2017/01/25 01:10:14, dominickn wrote: > On 2017/01/24 19:09:35, F wrote: > > Hi Dominick & Peter, PTAL. Thanks! > > Hi Felix, I'm just thinking about the overall design here. Are we likely to > fetch more icon types in the future? Perhaps splashscreen? If that's the case, > we might want to actually change the guts of InstallableManager slightly so that > at the start, we fill the icons_ vector with everything we want to fetch, then > we make a distinct FetchIcons() method called by WorkOnTask() that iterates over > icons_, fetching each one in there. That will scale better, and avoid some of > the repetitive params construction. > > Don't make this change yet - let me think about it a bit more. But knowing the > answer to that question will help. Hi Dominick, I thought about it, too. However, as far as I know, we have no plans for splashscreen-purposed icons in WebAPK. Also, the discussion on including more "purpose" died down on Web Manifest github, so I think it is safe to assume that the status quo of at most 2 icon purposes is going to stay for some time. Therefore, I tried to minimize the changes that I made to InstallableManager. I think it is ok to generalize it later. WDUT?
OK, thanks. We can leave the structure of the implementation as is then. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:304: if (!manifest_->fetched) Every line of this conditional needs braces now (there are multi-line else clauses later on). https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:309: !IsIconFetched(ParamsForPrimaryIcon(params))) This repeated construction of the params throughout this cc file is a little upsetting. It would be good if we could find a way to avoid that. Let me think on what a good solution might be. Possibly introducing a new internal struct for each Task (instead of reusing InstallableParams internally) so that we construct the params once and are done with it. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:441: // Filter by icon purpose. I think a better idea here is to make ManifestIconSelector take an icon purpose so we avoid this extra (needless) copy. Perhaps an initial CL that does that? (That also reminds me that I should get myself added to chrome/browser/manifest OWNERs at some point) https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:31: // current URL. The bools are last so that this struct will be laid out in memory more optimally. Can you please reorganise this such that the three bools are last? https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:89: // fetch_valid_badge_icon was true and a badge icon could not be retrieved, I don't think this comment is accurate - the badge icon is optional so there's never an error code for it. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:200: void OnAppIconFetched( Rename to OnIconFetched. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:234: EXPECT_TRUE(tester->primary_icon_url().is_empty()); Tests which don't fetch the badge should check that it is nullptr (many). https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:433: // Request an oversized badge icon. This should should only primary icon, but This comment is a bit weird
Thanks Dominick, PTAL! https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:304: if (!manifest_->fetched) On 2017/01/27 06:40:42, dominickn wrote: > Every line of this conditional needs braces now (there are multi-line else > clauses later on). Done. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:309: !IsIconFetched(ParamsForPrimaryIcon(params))) On 2017/01/27 06:40:42, dominickn wrote: > This repeated construction of the params throughout this cc file is a little > upsetting. It would be good if we could find a way to avoid that. Let me think > on what a good solution might be. Possibly introducing a new internal struct for > each Task (instead of reusing InstallableParams internally) so that we construct > the params once and are done with it. I agree that the repeated construction may seem a bit too much. However, I don't think it's a bad idea. Previously, each step would fetch the InstallableParams from the task queue, and invoke GetIcon, thereby constructing IconParams. The current approach is basically the same, except that it centralizes InstallableParams into WorkOnTask. I prefer reusing InstallableParams over introducing new struct. WDUT? https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:441: // Filter by icon purpose. On 2017/01/27 06:40:42, dominickn wrote: > I think a better idea here is to make ManifestIconSelector take an icon purpose > so we avoid this extra (needless) copy. Perhaps an initial CL that does that? > > (That also reminds me that I should get myself added to chrome/browser/manifest > OWNERs at some point) I agree that it is a better idea. Previously I explored this and found that ManifestIconSelector would also create an extra copy so eventually decided to put the extra copy here. I'm going to try combining the type filter and the purpose filter in ManifestIconSelector and let you decide :) https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:31: // current URL. On 2017/01/27 06:40:43, dominickn wrote: > The bools are last so that this struct will be laid out in memory more > optimally. > > Can you please reorganise this such that the three bools are last? Done. Thanks for the explanation. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:89: // fetch_valid_badge_icon was true and a badge icon could not be retrieved, On 2017/01/27 06:40:42, dominickn wrote: > I don't think this comment is accurate - the badge icon is optional so there's > never an error code for it. Done. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.h:200: void OnAppIconFetched( On 2017/01/27 06:40:43, dominickn wrote: > Rename to OnIconFetched. Done. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:234: EXPECT_TRUE(tester->primary_icon_url().is_empty()); On 2017/01/27 06:40:43, dominickn wrote: > Tests which don't fetch the badge should check that it is nullptr (many). Done. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:433: // Request an oversized badge icon. This should should only primary icon, but On 2017/01/27 06:40:43, dominickn wrote: > This comment is a bit weird Done. Thanks!
https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:309: !IsIconFetched(ParamsForPrimaryIcon(params))) Fair enough. Let's leave this as is for now. It's not *that* much more repetitions, and it's less elegant to handle this some other way. https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:441: // Filter by icon purpose. On 2017/01/27 20:11:17, F wrote: > On 2017/01/27 06:40:42, dominickn wrote: > > I think a better idea here is to make ManifestIconSelector take an icon > purpose > > so we avoid this extra (needless) copy. Perhaps an initial CL that does that? > > > > (That also reminds me that I should get myself added to > chrome/browser/manifest > > OWNERs at some point) > > I agree that it is a better idea. Previously I explored this and found that > ManifestIconSelector would also create an extra copy so eventually decided to > put the extra copy here. > > I'm going to try combining the type filter and the purpose filter in > ManifestIconSelector and let you decide :) I'm confused why ManifestIconSelector would make an extra copy. Why not change FindBestMatchingIcon to take an IconPurpose as an extra argument, and add an IconContainsRequiredPurpose() method that you check before accepting an icon? Is there something I'm missing? :) https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:89: // is optional, clients must check for nullptr even if fetch_valid_badge_icon Slight tweak to the comment: "Since the badge icon is optional, no error code is set if it cannot be fetched, and clients specifying fetch_valid_badge_icon must check that the bitmap exists before using it". https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:443: Minor nit: remove the line breaks in between icon and installable checks= so this matches all of the other tests in this file. https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:469: Ditto re line breaks
Hi Dominick, PTAL. Thanks! https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:441: // Filter by icon purpose. On 2017/01/30 00:51:26, dominickn wrote: > On 2017/01/27 20:11:17, F wrote: > > On 2017/01/27 06:40:42, dominickn wrote: > > > I think a better idea here is to make ManifestIconSelector take an icon > > purpose > > > so we avoid this extra (needless) copy. Perhaps an initial CL that does > that? > > > > > > (That also reminds me that I should get myself added to > > chrome/browser/manifest > > > OWNERs at some point) > > > > I agree that it is a better idea. Previously I explored this and found that > > ManifestIconSelector would also create an extra copy so eventually decided to > > put the extra copy here. > > > > I'm going to try combining the type filter and the purpose filter in > > ManifestIconSelector and let you decide :) > > I'm confused why ManifestIconSelector would make an extra copy. Why not change > FindBestMatchingIcon to take an IconPurpose as an extra argument, and add an > IconContainsRequiredPurpose() method that you check before accepting an icon? Is > there something I'm missing? :) At the moment, FilterIconsByType creates an extra copy of icons: https://cs.chromium.org/chromium/src/chrome/browser/manifest/manifest_icon_se... So previously I thought I might write a similar function FilterIconsByPurpose. I'm going to refactor ManifestIconSelector to get rid of the extra copy of icons. WDUT? https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:89: // is optional, clients must check for nullptr even if fetch_valid_badge_icon On 2017/01/30 00:51:27, dominickn wrote: > Slight tweak to the comment: > > "Since the badge icon is optional, no error code is set if it cannot be fetched, > and clients specifying fetch_valid_badge_icon must check that the bitmap exists > before using it". Done. https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:443: On 2017/01/30 00:51:27, dominickn wrote: > Minor nit: remove the line breaks in between icon and installable checks= so > this matches all of the other tests in this file. Done. https://codereview.chromium.org/2641413002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:469: On 2017/01/30 00:51:27, dominickn wrote: > Ditto re line breaks Done.
lgtm if you move the purpose filtering to ManifestIconSelector asap https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2641413002/diff/80001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:441: // Filter by icon purpose. On 2017/01/30 16:50:05, F wrote: > On 2017/01/30 00:51:26, dominickn wrote: > > On 2017/01/27 20:11:17, F wrote: > > > On 2017/01/27 06:40:42, dominickn wrote: > > > > I think a better idea here is to make ManifestIconSelector take an icon > > > purpose > > > > so we avoid this extra (needless) copy. Perhaps an initial CL that does > > that? > > > > > > > > (That also reminds me that I should get myself added to > > > chrome/browser/manifest > > > > OWNERs at some point) > > > > > > I agree that it is a better idea. Previously I explored this and found that > > > ManifestIconSelector would also create an extra copy so eventually decided > to > > > put the extra copy here. > > > > > > I'm going to try combining the type filter and the purpose filter in > > > ManifestIconSelector and let you decide :) > > > > I'm confused why ManifestIconSelector would make an extra copy. Why not change > > FindBestMatchingIcon to take an IconPurpose as an extra argument, and add an > > IconContainsRequiredPurpose() method that you check before accepting an icon? > Is > > there something I'm missing? :) > > At the moment, FilterIconsByType creates an extra copy of icons: > https://cs.chromium.org/chromium/src/chrome/browser/manifest/manifest_icon_se... > So previously I thought I might write a similar function FilterIconsByPurpose. > > I'm going to refactor ManifestIconSelector to get rid of the extra copy of > icons. WDUT? I didn't realise there was already a copy in there. There should be no reason for ManifestIconSelector to be making copies, so +1 to a refactor. I would prefer that you didn't have this additional copy here now, but if you do that refactor and add a purpose filter asap I'll be okay with that.
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zpeng@chromium.org
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zpeng@chromium.org
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2641413002/#ps180001 (title: "Fix rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485816049947550,
"parent_rev": "7bc8fc1007007ece8eaa4137967c3162bad4db08", "commit_rev":
"b205d0d471142ed307b73146e51e3576e4ffcd6e"}
Message was sent while issue was closed.
Description was changed from ========== Allow InstallableManager to fetch optional badge icon This CL updates InstallableManager so that users can specify badge-icon-related parameters in InstallableParams and retrieve the most suitable badge icon in InstallableData. Unlike the primary icon, badge icon is optional. Therefore, when a badge icon is requested yet no suitable icons are found in the web manifest, the GetData process would still succeed. The badge icon is to be used for Status Bar Icons for WebAPK's notifications. BUG=649771 ========== to ========== Allow InstallableManager to fetch optional badge icon This CL updates InstallableManager so that users can specify badge-icon-related parameters in InstallableParams and retrieve the most suitable badge icon in InstallableData. Unlike the primary icon, badge icon is optional. Therefore, when a badge icon is requested yet no suitable icons are found in the web manifest, the GetData process would still succeed. The badge icon is to be used for Status Bar Icons for WebAPK's notifications. BUG=649771 Review-Url: https://codereview.chromium.org/2641413002 Cr-Commit-Position: refs/heads/master@{#447154} Committed: https://chromium.googlesource.com/chromium/src/+/b205d0d471142ed307b73146e51e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b205d0d471142ed307b73146e51e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
