| 
 | 
 | 
 Chromium Code Reviews
 Chromium Code Reviews Issue 
            2820763002:
    Remove usage of Tab's |url| property from TabUsageRecorder.  (Closed)
    
  
    Issue 
            2820763002:
    Remove usage of Tab's |url| property from TabUsageRecorder.  (Closed) 
  | Created: 3 years, 8 months ago by kkhorimoto Modified: 3 years, 6 months ago CC: chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, asvitkine+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref: refs/heads/master Project: chromium Visibility: Public. | DescriptionRemove usage of Tab's |url| property from TabUsageRecorder.
BUG=546208
Review-Url: https://codereview.chromium.org/2820763002
Cr-Commit-Position: refs/heads/master@{#476528}
Committed: https://chromium.googlesource.com/chromium/src/+/8dd141c8941e66957a4627ae51b3918c9f457b20
   Patch Set 1 #Patch Set 2 : visible => lastCommitted #
      Total comments: 3
      
     Patch Set 3 : check pending url #Patch Set 4 : fixed tab mock #Patch Set 5 : fix gn #Patch Set 6 : rebase #
 Messages
    Total messages: 36 (19 generated)
     
 visible => lastCommitted 
 kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org, rohitrao@chromium.org, sdefresne@chromium.org 
 https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... File ios/chrome/browser/metrics/tab_usage_recorder.mm (right): https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... ios/chrome/browser/metrics/tab_usage_recorder.mm:297: return tab.lastCommittedURL.SchemeIs(kChromeUIScheme); chrome:// URLs are never pending, so lastCommitted should be sufficient. 
 https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... File ios/chrome/browser/metrics/tab_usage_recorder.mm (right): https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... ios/chrome/browser/metrics/tab_usage_recorder.mm:297: return tab.lastCommittedURL.SchemeIs(kChromeUIScheme); On 2017/04/14 20:40:47, kkhorimoto_ wrote: > chrome:// URLs are never pending, so lastCommitted should be sufficient. WebUI can be a pending URL, so maybe we should check both pending and last committed? 
 The CQ bit was checked by kkhorimoto@chromium.org to run a CQ dry run 
 Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... 
 The CQ bit was checked by kkhorimoto@chromium.org to run a CQ dry run 
 Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... 
 https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... File ios/chrome/browser/metrics/tab_usage_recorder.mm (right): https://codereview.chromium.org/2820763002/diff/20001/ios/chrome/browser/metr... ios/chrome/browser/metrics/tab_usage_recorder.mm:297: return tab.lastCommittedURL.SchemeIs(kChromeUIScheme); On 2017/04/14 21:48:09, Eugene But wrote: > On 2017/04/14 20:40:47, kkhorimoto_ wrote: > > chrome:// URLs are never pending, so lastCommitted should be sufficient. > WebUI can be a pending URL, so maybe we should check both pending and last > committed? Done. 
 lgtm 
 The CQ bit was unchecked by commit-bot@chromium.org 
 Dry run: 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...) 
 Looks like there are real crashes in TabUsageRecorderTest. Otherwise testing pending_item first sgtm. 
 fixed tab mock 
 The CQ bit was checked by kkhorimoto@chromium.org 
 The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2820763002/#ps60001 (title: "fixed tab mock") 
 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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 
 fix gn 
 The CQ bit was checked by kkhorimoto@chromium.org 
 The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2820763002/#ps80001 (title: "fix gn") 
 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 
 rebase 
 The CQ bit was checked by kkhorimoto@chromium.org 
 The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2820763002/#ps100001 (title: "rebase") 
 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 
 The CQ bit was checked by kkhorimoto@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": 100001, "attempt_start_ts": 1496356806868570,
"parent_rev": "97474434c7f06805f9bdc97c2848ef25cf07f550", "commit_rev":
"8dd141c8941e66957a4627ae51b3918c9f457b20"}
 
            
              
                Message was sent while issue was closed.
              
            
             Description was changed from ========== Remove usage of Tab's |url| property from TabUsageRecorder. BUG=546208 ========== to ========== Remove usage of Tab's |url| property from TabUsageRecorder. BUG=546208 Review-Url: https://codereview.chromium.org/2820763002 Cr-Commit-Position: refs/heads/master@{#476528} Committed: https://chromium.googlesource.com/chromium/src/+/8dd141c8941e66957a4627ae51b3... ========== 
 
            
              
                Message was sent while issue was closed.
              
            
             Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8dd141c8941e66957a4627ae51b3... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
