Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
933423002:
Push API: Don't reveal whether incognito mode is active. (Closed)
DescriptionPush API: Don't reveal whether incognito mode is active.
BUG=401439
Committed: https://crrev.com/4cf440af4e02b4b380a751db26faee10f2e7e524
Cr-Commit-Position: refs/heads/master@{#317108}
Patch Set 1 #Patch Set 2 : Fix typo (return -> break) #
Total comments: 3
Patch Set 3 : Explicitly check for incognito mode #
Total comments: 4
Patch Set 4 : Add TODO suggested by mlamouri #Messages
Total messages: 20 (7 generated)
johnme@chromium.org changed reviewers: + mlamouri@chromium.org
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Drive-by lgtm with nit https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:317: // Prevent websites from detecting incognito mode, by emulating what would As discussed offline, this case could conceivably happen in situations other than incognito, maybe some non-official-Chrome builds for example. Please add a TODO for explicitly checking for Incognito mode.
https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:317: // Prevent websites from detecting incognito mode, by emulating what would On 2015/02/19 at 11:15:28, Michael van Ouwerkerk wrote: > As discussed offline, this case could conceivably happen in situations other than incognito, maybe some non-official-Chrome builds for example. Please add a TODO for explicitly checking for Incognito mode. Why doing that in a TODO? Why not doing that in that CL directly? You could say that the permission is denied in incognito mode. Ideally, have a randomly delayed task to do so. If not in incognito mode and no service, we could throw.
As suggested, it now checks for incognito mode, and continues to throw otherwise. https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933423002/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:317: // Prevent websites from detecting incognito mode, by emulating what would On 2015/02/19 11:20:16, Mounir Lamouri wrote: > On 2015/02/19 at 11:15:28, Michael van Ouwerkerk wrote: > > As discussed offline, this case could conceivably happen in situations other > than incognito, maybe some non-official-Chrome builds for example. Please add a > TODO for explicitly checking for Incognito mode. > > Why doing that in a TODO? Why not doing that in that CL directly? You could say > that the permission is denied in incognito mode. Ideally, have a randomly > delayed task to do so. If not in incognito mode and no service, we could throw. Done.
New patchsets have been uploaded after l-g-t-m from mvanouwerkerk@chromium.org
Explicitly check for incognito mode
lgtm https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:325: BrowserThread::PostTask( TODO: we might want to not expose the API in that case. https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:343: // random time period. I think it would be great to fix this TODO soon after landing this CL.
lgtm
New patchsets have been uploaded after l-g-t-m from mlamouri@chromium.org,mvanouwerkerk@chromium.org
Thanks for reviews :) https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:325: BrowserThread::PostTask( On 2015/02/19 16:14:51, Mounir Lamouri wrote: > TODO: we might want to not expose the API in that case. Done. https://codereview.chromium.org/933423002/diff/40001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:343: // random time period. On 2015/02/19 16:14:51, Mounir Lamouri wrote: > I think it would be great to fix this TODO soon after landing this CL. Acknowledged.
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/933423002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)
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/933423002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4cf440af4e02b4b380a751db26faee10f2e7e524 Cr-Commit-Position: refs/heads/master@{#317108} |
