Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882183002:
Thumbnails: Add some perf UMA (Closed)
|
Created:
3 years, 7 months ago by Marc Treib Modified:
3 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThumbnails: Add some perf UMA
BUG=718413
Review-Url: https://codereview.chromium.org/2882183002
Cr-Commit-Position: refs/heads/master@{#472138}
Committed: https://chromium.googlesource.com/chromium/src/+/4a79f66a7649512ba0e1cebd1e1d3c3cccb1bdc1
Patch Set 1 #
Total comments: 5
Patch Set 2 : no more using #
Messages
Total messages: 27 (20 generated)
The CQ bit was checked by treib@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...
treib@chromium.org changed reviewers: + holte@chromium.org, jochen@chromium.org
jochen: Please look at thumbnail_tab_helper.h/cc. (Unfortunately, c/b/thumbnails doesn't have any direct owners.) holte: Please look at histograms.xml. Thanks! https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... File chrome/browser/thumbnails/thumbnail_tab_helper.cc (left): https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... chrome/browser/thumbnails/thumbnail_tab_helper.cc:212: void ThumbnailTabHelper::CleanUpFromThumbnailGeneration() { Moved to match the order in the header.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by treib@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
who would be a good owner for thumbnails code? https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... File chrome/browser/thumbnails/thumbnail_tab_helper.cc (left): https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... chrome/browser/thumbnails/thumbnail_tab_helper.cc:51: ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents) using content::Something should be the exception, so if you touch that, you should rather change towards the other direction https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right): https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... chrome/browser/thumbnails/thumbnail_tab_helper.cc:122: if (!web_contents() || web_contents()->IsBeingDestroyed()) { why this change? if (single line) single line; is already allowed
I don't know who'd be a good owner. From history, it seems this code hasn't been touched much in recent times, apart from mechanic changes. thestig@ didn't seem to mind reviewing a previous change here, but was OOO when I sent this out. And also didn't know who'd make a good owner. https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... File chrome/browser/thumbnails/thumbnail_tab_helper.cc (left): https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... chrome/browser/thumbnails/thumbnail_tab_helper.cc:51: ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents) On 2017/05/16 11:15:17, jochen wrote: > using content::Something should be the exception, so if you touch that, you > should rather change towards the other direction Done. (For all three "using content::"s) https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right): https://codereview.chromium.org/2882183002/diff/1/chrome/browser/thumbnails/t... chrome/browser/thumbnails/thumbnail_tab_helper.cc:122: if (!web_contents() || web_contents()->IsBeingDestroyed()) { On 2017/05/16 11:15:17, jochen wrote: > why this change? > > if (single line) > single line; > > is already allowed For local consistency, since there are a few other single-line bodies in this file that do have braces. If you prefer, I can change all of them to not have braces instead (or just undo these changes - I don't care *that* much).
lgtm
The CQ bit was checked by treib@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by treib@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 unchecked by treib@chromium.org
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org Link to the patchset: https://codereview.chromium.org/2882183002/#ps20001 (title: "no more using")
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": 1494952199785610,
"parent_rev": "606424bd7f6b579fdc99e1d230911b68147ecb31", "commit_rev":
"4a79f66a7649512ba0e1cebd1e1d3c3cccb1bdc1"}
Message was sent while issue was closed.
Description was changed from ========== Thumbnails: Add some perf UMA BUG=718413 ========== to ========== Thumbnails: Add some perf UMA BUG=718413 Review-Url: https://codereview.chromium.org/2882183002 Cr-Commit-Position: refs/heads/master@{#472138} Committed: https://chromium.googlesource.com/chromium/src/+/4a79f66a7649512ba0e1cebd1e1d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4a79f66a7649512ba0e1cebd1e1d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
