Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
866443003:
Push API: Don't require notification if tab is visible. (Closed)
|
Created:
5 years, 11 months ago by johnme Modified:
5 years, 10 months ago Reviewers:
David Trainor- moved to gerrit, mlamouri (slow - plz ping), jochen (gone - plz use gerrit), Mr4D (OOO till 08-26), Michael van Ouwerkerk CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ycm_outdir Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPush API: Don't require notification if tab is visible.
For developers who have opted in to showing a user-visible UI change on every push (in exchange for showing the user a less scary permission prompt), we currently[1] enforce that each push shows a notification.
This patch relaxes the requirement slightly, so that sites don't need to show user-visible UI if a same-origin-ish[2] tab is currently visible, i.e. is the active tab of a non-minimized window. That's because updating the UI of such a tab instead of showing a notification is a valid way to show user-visible UI.
See also:
- https://crbug.com/437277
- http://lists.w3.org/Archives/Public/public-webapps/2014OctDec/0214.html
- https://github.com/w3c/push-api/pull/87
[1]: (since https://codereview.chromium.org/842233003)
[2]: (see comment in push_messaging_service_impl.cc)
BUG=437277
Committed: https://crrev.com/266a9e0aa173d6bffbc6484d09c6407c096e6ac3
Cr-Commit-Position: refs/heads/master@{#314586}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added push_messaging_service.cc #
Total comments: 1
Patch Set 3 : Address Michael's review comments #
Total comments: 4
Patch Set 4 : Add {} #Patch Set 5 : Expose WebContents::IsHiddenIgnoringCapturing instead of PushMessagingService::IsWebContentsUserVis… #
Total comments: 1
Patch Set 6 : Use active_web_contents->GetMainFrame()->GetVisibilityState() #
Total comments: 2
Patch Set 7 : Switch statement for GetVisibilityState #
Total comments: 16
Patch Set 8 : Addressed Mounir's review comments #
Total comments: 2
Patch Set 9 : Fix Android compile #
Messages
Total messages: 40 (9 generated)
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:269: // as even with navigator.connect the Service Worker can't talk to it. Comment seems long. https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:275: active_web_contents->GetVisibleURL(), Please document why GetLastCommittedUrl is the wrong thing here. https://codereview.chromium.org/866443003/diff/1/chrome/browser/ui/android/ta... File chrome/browser/ui/android/tab_model/tab_model.cc (right): https://codereview.chromium.org/866443003/diff/1/chrome/browser/ui/android/ta... chrome/browser/ui/android/tab_model/tab_model.cc:17: static int INVALID_TAB_INDEX = -1; Similar comment as in TabList.java? https://codereview.chromium.org/866443003/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/866443003/diff/1/content/content_browser.gypi... content/content_browser.gypi:180: 'public/browser/push_messaging_service.cc', Let's also add this file to the CL :-)
https://codereview.chromium.org/866443003/diff/20001/content/public/browser/p... File content/public/browser/push_messaging_service.cc (right): https://codereview.chromium.org/866443003/diff/20001/content/public/browser/p... content/public/browser/push_messaging_service.cc:12: bool PushMessagingService::IsWebContentsUserVisible( Shouldn't this just be a method of WebContents?
https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:269: // as even with navigator.connect the Service Worker can't talk to it. On 2015/01/21 19:13:22, Michael van Ouwerkerk wrote: > Comment seems long. What I'm doing here is a bit unusual: I'm requiring the scheme to match, but not for security reasons. Hence the wordy explanation. https://codereview.chromium.org/866443003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:275: active_web_contents->GetVisibleURL(), On 2015/01/21 19:13:22, Michael van Ouwerkerk wrote: > Please document why GetLastCommittedUrl is the wrong thing here. Done. https://codereview.chromium.org/866443003/diff/1/chrome/browser/ui/android/ta... File chrome/browser/ui/android/tab_model/tab_model.cc (right): https://codereview.chromium.org/866443003/diff/1/chrome/browser/ui/android/ta... chrome/browser/ui/android/tab_model/tab_model.cc:17: static int INVALID_TAB_INDEX = -1; On 2015/01/21 19:13:22, Michael van Ouwerkerk wrote: > Similar comment as in TabList.java? Done. https://codereview.chromium.org/866443003/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/866443003/diff/1/content/content_browser.gypi... content/content_browser.gypi:180: 'public/browser/push_messaging_service.cc', On 2015/01/21 19:13:22, Michael van Ouwerkerk wrote: > Let's also add this file to the CL :-) Done.
johnme@chromium.org changed reviewers: + avi@chromium.org, dtrainor@chromium.org
dtrainor@chromium.org: please review TabList.java and tab_model.{h,cc} avi@chromium.org: please review content/, especially push_messaging_service.cc which feels a little hacky, though I don't see a good alternative other than adding WebContentsImpl::should_normally_be_visible() to WebContents's public API Thanks!
johnme@chromium.org changed reviewers: - avi@chromium.org
johnme@chromium.org changed reviewers: + jochen@chromium.org
Swapping avi@ for jochen@ as he's OOO. jochen@chromium.org: please review content/, especially push_messaging_service.cc which feels a little hacky, though I don't see a good alternative other than adding WebContentsImpl::should_normally_be_visible() to WebContents's public API Thanks!
i don't really understand what this change is supposed to do? Can you elaborate on this also in the CL description? https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: !profile->IsSameProfile(profile_)) nit, add {} https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:278: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) add {}
On 2015/01/22 16:23:19, jochen (slow) wrote: > i don't really understand what this change is supposed to do? > > Can you elaborate on this also in the CL description? I expanded the description a lot - hopefully it's clearer now. You may also be interested in this draft doc where we're trying to sketch out the exact heuristics to use: https://docs.google.com/document/d/1WtORlXmdnziLJTpaUfI_cd-_JEVNdFvlUEh_dusq6... https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: !profile->IsSameProfile(profile_)) On 2015/01/22 16:23:19, jochen (slow) wrote: > nit, add {} Done. https://codereview.chromium.org/866443003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:278: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) On 2015/01/22 16:23:19, jochen (slow) wrote: > add {} Done.
chrome/android/java and chrome/browser/ui/android lgtm. I don't know about the feature/code area for the rest so I'll defer to the other reviewers for those files.
jochen@chromium.org changed reviewers: + skuhne@chromium.org
the comment on WebContentsImpl::should_normally_be_visible_ makes me wonder whether that's really what you want. +skuhne, who might know In any case, I think if this is what you want, it should be exposed via WebContents instead of PushMessagingService.
On 2015/01/23 14:16:34, jochen (slow) wrote: > the comment on WebContentsImpl::should_normally_be_visible_ makes me wonder > whether that's really what you want. +skuhne, who might know I want to match the Page Visibility API (so that websites can accurately predict what will happen). The call stack which sets page visibility is: blink::Page::setVisibilityState(blink::PageVisibilityState, bool) third_party/WebKit/Source/core/page/Page.cpp:424 - Called by: blink::WebViewImpl::setVisibilityState(blink::WebPageVisibilityState, bool) - third_party/WebKit/Source/web/WebViewImpl.cpp:4479 - - Called by: content::RenderViewImpl::OnWasHidden() - - content/renderer/render_view_impl.cc:3389 - - - Overrides: content::RenderWidget::OnWasHidden() - - - content/renderer/render_widget.h:410 - - - - Referenced at: IPC_MESSAGE_HANDLER(ViewMsg_WasHidden, OnWasHidden) - - - - content/renderer/render_widget.cc:699 - - - - - IPC message: ViewMsg_WasHidden - - - - - content/common/view_messages.h:628 - - - - - - Instantiated by: content::RenderWidgetHostImpl::WasHidden() - - - - - - content/browser/renderer_host/render_widget_host_impl.cc:501 - - - - - - - Called by: content::RenderWidgetHostViewAura::Hide() - - - - - - - content/browser/renderer_host/render_widget_host_view_aura.cc:614 - - - - - - - - Called by: content::WebContentsImpl::WasHidden() - - - - - - - - content/browser/web_contents/web_contents_impl.cc:1071 And similarly for WasShown. So to observe which of WebContents::WasShown and WebContents::WasHidden were called more recently, it's should_normally_be_visible() that I need access to, not IsHidden(). There's perhaps a valid debate to be had about whether a tab that's being mirrored over Cast or similar to an external display should be considered visible or hidden by the Page Visibility API, but for now they don't seem to be. > In any case, I think if this is what you want, it should be exposed via > WebContents instead of PushMessagingService. Fair - that's what I was wondering. What should I call the method on WebContents? should_normally_be_visible isn't particularly clear. How about IsHiddenIgnoringCapturing()?
On 2015/01/23 14:16:34, jochen (slow) wrote: > In any case, I think if this is what you want, it should be exposed via > WebContents instead of PushMessagingService. Done. I called it WebContents::IsHiddenIgnoringCapturing to avoid confusion with WebContentsImpl::IsHidden (which should perhaps be renamed IsHiddenAndNotBeingCaptured?).
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/866443003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:275: // as even with navigator.connect the Service Worker can't talk to it. So that means that if I have one https://<username>.github.io/<repo> open, any of those could use silent push?
On 2015/01/26 11:53:35, Mounir Lamouri wrote: > https://codereview.chromium.org/866443003/diff/80001/chrome/browser/services/... > chrome/browser/services/gcm/push_messaging_service_impl.cc:275: // as even with > navigator.connect the Service Worker can't talk to it. > So that means that if I have one https://<username>.github.io/<repo> open, any > of those could use silent push? Not really, for a couple of reasons: - I'm checking for pages being visible, not open (though it's possible that'll change one day). - github.io is listed in https://publicsuffix.org/list/effective_tld_names.dat and I pass net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES, so foo.github.io != bar.github.io (but a.foo.github.com == b.foo.github.com). - It's not generally possible to tell what cross-origin websites are currently visible (or open for that matter), so you wouldn't be able to reliably exploit this (to avoid showing notifications) anyway.
i wonder whether it's possible to base this on top of https://codereview.chromium.org/871443004 ?
On 2015/01/26 at 16:19:29, jochen wrote: > i wonder whether it's possible to base this on top of https://codereview.chromium.org/871443004 ? If I expose GetPageVisibilityState() to RFH (only in RFHI in my CL), this CL should be able to use it by doing: active_web_contents->GetMainFrame()->GetPageVisibilityState();
On 2015/01/26 16:27:03, Mounir Lamouri wrote: > On 2015/01/26 at 16:19:29, jochen wrote: > > i wonder whether it's possible to base this on top of > https://codereview.chromium.org/871443004 ? > > If I expose GetPageVisibilityState() to RFH (only in RFHI in my CL), this CL > should be able to use it by doing: > active_web_contents->GetMainFrame()->GetPageVisibilityState(); Yeah, that would a neat alternative. Jochen, does that sound good to you?
On 2015/01/26 at 16:57:43, johnme wrote: > On 2015/01/26 16:27:03, Mounir Lamouri wrote: > > On 2015/01/26 at 16:19:29, jochen wrote: > > > i wonder whether it's possible to base this on top of > > https://codereview.chromium.org/871443004 ? > > > > If I expose GetPageVisibilityState() to RFH (only in RFHI in my CL), this CL > > should be able to use it by doing: > > active_web_contents->GetMainFrame()->GetPageVisibilityState(); > > Yeah, that would a neat alternative. Jochen, does that sound good to you? I updated the CL to expose GetVisibilityState() in RFH. Maybe you could update that CL to be based on top of it?
On 2015/01/26 at 16:57:43, johnme wrote: > On 2015/01/26 16:27:03, Mounir Lamouri wrote: > > On 2015/01/26 at 16:19:29, jochen wrote: > > > i wonder whether it's possible to base this on top of > > https://codereview.chromium.org/871443004 ? > > > > If I expose GetPageVisibilityState() to RFH (only in RFHI in my CL), this CL > > should be able to use it by doing: > > active_web_contents->GetMainFrame()->GetPageVisibilityState(); > > Yeah, that would a neat alternative. Jochen, does that sound good to you? sgtm
On 2015/01/26 17:42:06, Mounir Lamouri wrote: > On 2015/01/26 at 16:57:43, johnme wrote: > > On 2015/01/26 16:27:03, Mounir Lamouri wrote: > > > On 2015/01/26 at 16:19:29, jochen wrote: > > > > i wonder whether it's possible to base this on top of > > > https://codereview.chromium.org/871443004 ? > > > > > > If I expose GetPageVisibilityState() to RFH (only in RFHI in my CL), this CL > > > should be able to use it by doing: > > > active_web_contents->GetMainFrame()->GetPageVisibilityState(); > > > > Yeah, that would a neat alternative. Jochen, does that sound good to you? > > I updated the CL to expose GetVisibilityState() in RFH. Maybe you could update > that CL to be based on top of it? Done, PTAL. Thanks :)
https://codereview.chromium.org/866443003/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/100001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:278: != blink::WebPageVisibilityStateVisible) I thought I had left a comment about that yesterday but can't find it anymore... I think you should use a switch here to make sure you explicitly handle all the cases. For example, should prerending tabs be considered as visible?
https://codereview.chromium.org/866443003/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/100001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:278: != blink::WebPageVisibilityStateVisible) On 2015/01/28 09:26:57, Mounir Lamouri wrote: > I thought I had left a comment about that yesterday but can't find it anymore... > > I think you should use a switch here to make sure you explicitly handle all the > cases. For example, should prerending tabs be considered as visible? Done.
Looks good. Should be fine with the comments applied. https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_browsertest.cc:175: } I think the coding style recommends to not have default arguments (which you do here even if not explicit). Could you change all the call sites if that's not too crazy? (I guess sed could do that for you.) https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:271: !profile->IsSameProfile(profile_)) { These checks do not really match the comment. Only one does. What about doing the sanity check first: if (!profile || !active_web_contents) continue; Then, you could check that we are not leaking information: if (!profile->IsSameProfile(profile_)) continue; Regarding the last check, I'm not sure why it's needed. I assume it's arbitrary/temporary. If that's the case, could you point to a bug and add a TODO? https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:276: if (!active_web_contents) Again, the check doesn't match the comment. Could you merge that with the check of |profile| being not null. https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:293: // as even with navigator.connect the Service Worker can't talk to it. I wouldn't mention navigator.connect in the comments given that it's not something we ship (so there is a chance that the comment becomes irrelevant). https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1028: ditto https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (left): https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:184: nit: that change was probably not intentional, right? https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:184: nit: that change was probably not intentional, right?
Addressed Mounir's review comments https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_browsertest.cc:175: } On 2015/02/02 12:36:57, Mounir Lamouri wrote: > I think the coding style recommends to not have default arguments (which you do > here even if not explicit). Could you change all the call sites if that's not > too crazy? (I guess sed could do that for you.) According to http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum... it seems the only concern is that default arguments makes function pointers confusing, but the guide seems fine with using function overloading to simulate default arguments. Given that there are 46 calls to RunScript, many of which close to the line length limit, I think it would decrease legibility to add a "nullptr" argument to each of them. https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:271: !profile->IsSameProfile(profile_)) { On 2015/02/02 12:36:57, Mounir Lamouri wrote: > These checks do not really match the comment. Only one does. > > What about doing the sanity check first: > if (!profile || !active_web_contents) > continue; > > Then, you could check that we are not leaking information: > if (!profile->IsSameProfile(profile_)) > continue; > > Regarding the last check, I'm not sure why it's needed. I assume it's > arbitrary/temporary. If that's the case, could you point to a bug and add a > TODO? Done (I changed it to profile_!=profile, since IsSameProfile considers incognito and non-incognito to be the same which is inappropriate here, but pointer equality avoids that problem). https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:276: if (!active_web_contents) On 2015/02/02 12:36:58, Mounir Lamouri wrote: > Again, the check doesn't match the comment. Could you merge that with the check > of |profile| being not null. Done (split this out). https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:293: // as even with navigator.connect the Service Worker can't talk to it. On 2015/02/02 12:36:57, Mounir Lamouri wrote: > I wouldn't mention navigator.connect in the comments given that it's not > something we ship (so there is a chance that the comment becomes irrelevant). Chrome already implements most of navigator.connect (see content/{child,browser}/navigator_connect etc), so it seems reasonable to refer to it in comments. Did a minor rewording for clarity though. https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1028: On 2015/02/02 12:36:58, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (left): https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:184: On 2015/02/02 12:36:58, Mounir Lamouri wrote: > nit: that change was probably not intentional, right? Done. https://codereview.chromium.org/866443003/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:184: On 2015/02/02 12:36:58, Mounir Lamouri wrote: > nit: that change was probably not intentional, right? Done.
non-owner lgtm https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_browsertest.cc:175: } On 2015/02/02 at 15:58:36, johnme wrote: > On 2015/02/02 12:36:57, Mounir Lamouri wrote: > > I think the coding style recommends to not have default arguments (which you do > > here even if not explicit). Could you change all the call sites if that's not > > too crazy? (I guess sed could do that for you.) > > According to http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum... it seems the only concern is that default arguments makes function pointers confusing, but the guide seems fine with using function overloading to simulate default arguments. > > Given that there are 46 calls to RunScript, many of which close to the line length limit, I think it would decrease legibility to add a "nullptr" argument to each of them. I was once told to do exactly that but if the OWNER is fine with it, I do not mind. https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:293: // as even with navigator.connect the Service Worker can't talk to it. On 2015/02/02 at 15:58:36, johnme wrote: > On 2015/02/02 12:36:57, Mounir Lamouri wrote: > > I wouldn't mention navigator.connect in the comments given that it's not > > something we ship (so there is a chance that the comment becomes irrelevant). > > Chrome already implements most of navigator.connect (see content/{child,browser}/navigator_connect etc), so it seems reasonable to refer to it in comments. Did a minor rewording for clarity though. Chrome implementing it doesn't mean it will ship and will stay a thing. WebIntents has been removed for example. I do not mind if you want to keep that though.
lgtm with nit https://codereview.chromium.org/866443003/diff/140001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:244: // TODO(johnme): Relax this heuristic slightly. Is this TODO fixed now?
Now that this no longer touches content/, jochen said he's happy to leave the owners of the various chrome/ parts review this. Thanks all :) https://codereview.chromium.org/866443003/diff/140001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/866443003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:244: // TODO(johnme): Relax this heuristic slightly. On 2015/02/03 15:15:54, Michael van Ouwerkerk wrote: > Is this TODO fixed now? No, also needs https://codereview.chromium.org/883743002
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866443003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866443003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/266a9e0aa173d6bffbc6484d09c6407c096e6ac3 Cr-Commit-Position: refs/heads/master@{#314586} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
