New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 662978 link

Starred by 1 user
Status: WontFix
Owner:
█ awdf@chromium.org
Last visit > 30 days ago
Closed: Jan 2018
Cc:
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

ReadUserData should not return all-or-nothing

Project Member Reported by awdf@chromium.org, Nov 7 2016 
Right now ReadUserData either returns values for every key requested or none, if any cannot be found:

https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_database.h?sq=package:chromium&rcl=1478435172&l=181

This means we end up re-querying for sender id here:
https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_message_filter.cc?q=push_messaging_me&sq=package:chromium&l=323

The result is code that is not immediately understandable - it is necessary to go and look up the fact that ReadUserData is all-or-nothing to check the right thing is happening. If ReadUserData was not all or nothing, but instead returned whatever exists, then it would be obvious what's going on, and would avoid checking twice for the same data.

Other call-sites may require more checks of the return values = if necessary an 'allOrNothing' flag could be added for their convenience.

The question now becomes what status to return if only some keys exist, and what to return in place of values that do not exist. Is the empty string sufficient?
Project Member

Comment 1 by sheriffbot@chromium.org, Nov 8 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 2 by na...@chromium.org, Jan 15 2018

Owner: awdf@chromium.org
Status: Assigned (was: Untriaged)
It appears that the code has changed since this bug was first filed. For instance, I can't find the second file from Comment 1:
https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_message_filter.cc?q=push_messaging_me&sq=package:chromium&l=323

Also, the fact that this has been sitting in our queue for over a year might indicate it's no longer relevant.

I have a suspicion this logic has been migrated to Mojo.
Assigning to Anita to re-evaluate. Anita, feel free to assign to me if this is a low priority code cleanup and I can help. Thanks.

Comment 3 by awdf@chromium.org, Jan 15 2018

Cc: -joh...@chromium.org -harkness@chromium.org
Status: WontFix (was: Assigned)
The code has shifted but we still requery for the sender id from the PushMessagingManager, here: https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_manager.cc?l=400

That said, I think this can be closed as WontFix, it's only piece of code which would benefit and plenty more that would need to be fixed up, so in balance this ain't worth it given competing priorities.

Comment 4 by awdf@chromium.org, Jan 15 2018 

*only 1 piece of code
►

Sign in to add a comment