|
|
Created:
5 years, 11 months ago by benwells Modified:
5 years, 11 months ago Reviewers:
Matt Giuca CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeep the app list start page isolated from the browser's default zoom.
BUG=447062
Committed: https://crrev.com/e418076d38fade53c97df5f8175d32a1acae88ba
Cr-Commit-Position: refs/heads/master@{#312560}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Formatting #
Messages
Total messages: 19 (7 generated)
benwells@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:453: // DidNavigateMainFrame handler. Weird place for a comment (don't usually document overrides). Can you just put it inside? https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:456: const content::FrameNavigateParams& /* params */ ) { Nit: No spaces in between the /* and */ (see style guide). https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:461: // Set to have a zoom level of '0', which corresponds to 100%, so the Nit: Remove quotes around '0' (it is a number, not a char). https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:498: contents_.reset(); I think you need to add: Observe(nullptr); BEFORE this line --- otherwise you will have a dangling pointer to the deleted contents_. https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.h:122: const content::FrameNavigateParams& params) override; Very weird wrapping; has this been clang formatted?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:453: // DidNavigateMainFrame handler. On 2015/01/20 07:15:01, Matt Giuca wrote: > Weird place for a comment (don't usually document overrides). Can you just put > it inside? Done. https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:456: const content::FrameNavigateParams& /* params */ ) { On 2015/01/20 07:15:01, Matt Giuca wrote: > Nit: No spaces in between the /* and */ (see style guide). Done. https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:461: // Set to have a zoom level of '0', which corresponds to 100%, so the On 2015/01/20 07:15:01, Matt Giuca wrote: > Nit: Remove quotes around '0' (it is a number, not a char). Done. https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:498: contents_.reset(); On 2015/01/20 07:15:01, Matt Giuca wrote: > I think you need to add: > Observe(nullptr); > > BEFORE this line --- otherwise you will have a dangling pointer to the deleted > contents_. I don't think it's needed. See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... and also https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro.... https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.h:122: const content::FrameNavigateParams& params) override; On 2015/01/20 07:15:01, Matt Giuca wrote: > Very weird wrapping; has this been clang formatted? Oops, I thought I formatted it but turns out I didn't. Fixed.
https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:498: contents_.reset(); > I don't think it's needed. See > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... That removes the pointers from the WebContents to this WebContentsObserver, but there is still a raw pointer from this WebContentsObserver back to the now-deleted WebContents (WebContentsObserver::web_contents_). > and also > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro.... Ah, I didn't know about ResetWebContents: looks like that is exactly equivalent to Observe(nullptr) which I suggested. But it isn't being called from anywhere as far as I can tell (contents_.reset() is resetting the scoped_ptr, not calling WebContentsObserver::ResetWebContents). So yeah, I still think you need to call ResetWebContents() explicitly (do that instead of more awkwardly calling Observe(nullptr) which I previously suggested), before contents_.reset().
lgtm https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/863603003/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:498: contents_.reset(); Oops, as discussed offline, the WebContents destructor calls WebContentsObserver::ResetWebContents so this is not needed here. my bad
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/863603003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/863603003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/863603003/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e418076d38fade53c97df5f8175d32a1acae88ba Cr-Commit-Position: refs/heads/master@{#312560} |