Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513403003:
Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper (Closed)
DescriptionExplicitely pass ownership of ReadingListWebStateObserverUserDataWrapper
BUG=None
Committed: https://crrev.com/34145bb8726255a7da92275c2bb7bc40dda1ec14
Cr-Commit-Position: refs/heads/master@{#433821}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comment. #
Total comments: 2
Messages
Total messages: 16 (9 generated)
Description was changed from ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None ========== to ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None ==========
jif@chromium.org changed reviewers: + olivierrobin@chromium.org
is that better?
lgtm https://codereview.chromium.org/2513403003/diff/1/ios/chrome/browser/reading_... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2513403003/diff/1/ios/chrome/browser/reading_... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:40: wrapper = newDatWrapper.get(); newDataWrapper
jif@google.com changed reviewers: + jif@google.com
https://codereview.chromium.org/2513403003/diff/1/ios/chrome/browser/reading_... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2513403003/diff/1/ios/chrome/browser/reading_... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:40: wrapper = newDatWrapper.get(); On 2016/11/21 19:42:20, Olivier Robin wrote: > newDataWrapper Done.
The CQ bit was checked by jif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2513403003/#ps20001 (title: "Addressed comment.")
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": 20001, "attempt_start_ts": 1479805913344540,
"parent_rev": "f739e5564aabbb8ecb54f924d4b257e1ac40eb0c", "commit_rev":
"6ab8048e6843838b653dfa371e7ab7f11da93016"}
Message was sent while issue was closed.
Description was changed from ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None ========== to ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None ========== to ========== Explicitely pass ownership of ReadingListWebStateObserverUserDataWrapper BUG=None Committed: https://crrev.com/34145bb8726255a7da92275c2bb7bc40dda1ec14 Cr-Commit-Position: refs/heads/master@{#433821} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/34145bb8726255a7da92275c2bb7bc40dda1ec14 Cr-Commit-Position: refs/heads/master@{#433821}
Message was sent while issue was closed.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
Message was sent while issue was closed.
drive-by https://codereview.chromium.org/2513403003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2513403003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:38: auto newDataWrapper = style: this is a C++ class, so the variable should use_cpp_naming_conventions. On a higher level, most of the WebStateUserData inherits from web::WebStateUserData (see ios/web/public/web_state/web_state_user_data.h). Maybe this one should too. I don't see when this object is created as ReadingListWebStateObserver::FromWebState() seems to not yet be called (neither upstream nor downstream) so I don't know if the lazily creation is a good model for this object (i.e. code does not care whether this object exists before the first interaction) but from its name it look like it may be interested in capturing events as soon as the WebState is created. If this is the case, then it is better to have an explicit construction (look for CreateForWebState) and call it from the same location as the other WebStateUserData (currently in downstream code). https://codereview.chromium.org/2513403003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:62: ReadingListWebStateObserver* ReadingListWebStateObserver::FromWebState( This needs a comment to remind reader this is a static method: // static |
