Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784843002:
Reading List: Mark entry read when opening external app. (Closed)
|
Created:
3 years, 8 months ago by Olivier Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, stkhapugin, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReading List: Mark entry read when opening external app.
When opening an external app, the navigation is cancelled.
Report the opening of External app to the ReadingListWebStateObserver
and mark the entry read.
BUG=705964
Review-Url: https://codereview.chromium.org/2784843002
Cr-Commit-Position: refs/heads/master@{#461129}
Committed: https://chromium.googlesource.com/chromium/src/+/664989aae268019ac64667f276a74dd89a5185fb
Patch Set 1 #Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : feedback #
Total comments: 6
Patch Set 4 : feedback #
Messages
Total messages: 24 (10 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, jif@chromium.org
https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_web_delegate.h (right): https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_web_delegate.h:31: - (void)currentNavigationWillContinueInExternalApp; We should really avoid adding new methods to CRWWebDelegate (we working on removing this protocol). CRWWebDelegate already has 4 methods for launching external apps, so can we reuse |openExternalURL:linkClicked:| instead of adding a new one?
https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_web_delegate.h (right): https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_web_delegate.h:31: - (void)currentNavigationWillContinueInExternalApp; On 2017/03/29 15:44:15, Eugene But wrote: > We should really avoid adding new methods to CRWWebDelegate (we working on > removing this protocol). CRWWebDelegate already has 4 methods for launching > external apps, so can we reuse |openExternalURL:linkClicked:| instead of adding > a new one? |openExternalURL:linkClicked:| is called after [self stopLoading];in the web controller. So data on the current navigation is lost (and if the current page is native, like an ntp, a navigation to chrome://newtab is synchronously triggered). So to reuse |openExternalURL:linkClicked:|, we should either save and pass the data of the previous navigation, or move the call to [self stopLoading]; after the call to |openExternalURL:linkClicked:|. If you think one of these changes is safer, I can do it.
On 2017/03/29 15:49:45, Olivier Robin wrote: > https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... > File ios/web/public/web_state/ui/crw_web_delegate.h (right): > > https://codereview.chromium.org/2784843002/diff/20001/ios/web/public/web_stat... > ios/web/public/web_state/ui/crw_web_delegate.h:31: - > (void)currentNavigationWillContinueInExternalApp; > On 2017/03/29 15:44:15, Eugene But wrote: > > We should really avoid adding new methods to CRWWebDelegate (we working on > > removing this protocol). CRWWebDelegate already has 4 methods for launching > > external apps, so can we reuse |openExternalURL:linkClicked:| instead of > adding > > a new one? > > |openExternalURL:linkClicked:| is called after [self stopLoading];in the web > controller. > > So data on the current navigation is lost (and if the current page is native, > like an ntp, a navigation to chrome://newtab is synchronously triggered). > > So to reuse |openExternalURL:linkClicked:|, we should either save and pass the > data of the previous navigation, or move the call to [self stopLoading]; after > the call to |openExternalURL:linkClicked:|. > > If you think one of these changes is safer, I can do it. Passing new arguments to |openExternalURL:linkClicked:| sounds like a good idea. Changing the order of any calls can lead to unpredictable things (I don't have sufficient mental capacity to reason about any changes in External App Launching logic :().
Patchset #3 (id:40001) has been deleted
PTAL. Is this approach better?
Better indeed. Thanks and lgtm https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:353: //- (BOOL)openExternalURL:(const GURL&)url linkClicked:(BOOL)linkClicked; Looks like this is redefinition of CRWWebDelegate method. The issue is that CRWWebDelegate methods are not named like delegate methods, hence the collision. Both Tab and CRWWebDelegate will be removed, so you can just remove this predeclaration. https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1331: - (void)currentNavigationWillContinueInExternalApp { I guess you don't need this anymore
lgtm https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:352: //// external app. s/////////
Thanks https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:352: //// external app. On 2017/03/30 09:05:56, jif wrote: > s///////// This is removed https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:353: //- (BOOL)openExternalURL:(const GURL&)url linkClicked:(BOOL)linkClicked; On 2017/03/29 19:56:13, Eugene But wrote: > Looks like this is redefinition of CRWWebDelegate method. The issue is that > CRWWebDelegate methods are not named like delegate methods, hence the collision. > Both Tab and CRWWebDelegate will be removed, so you can just remove this > predeclaration. Done. https://codereview.chromium.org/2784843002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1331: - (void)currentNavigationWillContinueInExternalApp { On 2017/03/29 19:56:13, Eugene But wrote: > I guess you don't need this anymore Done.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2784843002/#ps80001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by olivierrobin@chromium.org
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": 80001, "attempt_start_ts": 1490972093293860,
"parent_rev": "70b42e102656627f3bd5259d0364f00c88f1c8f5", "commit_rev":
"664989aae268019ac64667f276a74dd89a5185fb"}
Message was sent while issue was closed.
Description was changed from ========== Reading List: Mark entry read when opening external app. When opening an external app, the navigation is cancelled. Report the opening of External app to the ReadingListWebStateObserver and mark the entry read. BUG=705964 ========== to ========== Reading List: Mark entry read when opening external app. When opening an external app, the navigation is cancelled. Report the opening of External app to the ReadingListWebStateObserver and mark the entry read. BUG=705964 Review-Url: https://codereview.chromium.org/2784843002 Cr-Commit-Position: refs/heads/master@{#461129} Committed: https://chromium.googlesource.com/chromium/src/+/664989aae268019ac64667f276a7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/664989aae268019ac64667f276a7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
