Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(154)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1376783004: Android: Mark more classes as non-mobile. (Closed)

Created:
5 years, 2 months ago by Lei Zhang
Modified:
5 years, 2 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Android: Mark more classes as non-mobile. - TabManager - FirstWebContentsProfiler - first run upgrade_util BUG=159847 TBR=isherman@chromium.org Committed: https://crrev.com/6ad7fe5183699ee528f9e7961cdef3124b1144fb Cr-Commit-Position: refs/heads/master@{#351738}

Patch Set 1 #

Total comments: 9

Patch Set 2 : address comments #

Total comments: 1

Patch Set 3 : fix build #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -30 lines) Patch
M chrome/browser/browser_process_impl.cc View 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/browser_shutdown.cc View 1 3 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/first_run/upgrade_util.h View 3 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h View 1 5 chunks +14 lines, -5 lines 1 comment Download
M chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc View 1 3 chunks +6 lines, -4 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 chunks +10 lines, -10 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 18 (4 generated)
Lei Zhang
georgesak: tab manager isherman: metrics gab: the rest https://codereview.chromium.org/1376783004/diff/1/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (left): https://codereview.chromium.org/1376783004/diff/1/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc#oldcode346 chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:346: first_web_contents_profiler_ ...
5 years, 2 months ago (2015-09-30 06:40:10 UTC) #2
gab
lgtm w/ nits/questions https://codereview.chromium.org/1376783004/diff/1/chrome/browser/browser_shutdown.cc File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/1376783004/diff/1/chrome/browser/browser_shutdown.cc#newcode252 chrome/browser/browser_shutdown.cc:252: #elif defined(OS_LINUX) Should this be OS_POSIX? ...
5 years, 2 months ago (2015-09-30 14:39:43 UTC) #3
Georges Khalil
LGTM with comments. https://codereview.chromium.org/1376783004/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1376783004/diff/1/chrome/chrome_browser.gypi#newcode649 chrome/chrome_browser.gypi:649: 'browser/memory/tab_stats.h', We can move all the ...
5 years, 2 months ago (2015-09-30 15:04:52 UTC) #4
Lei Zhang
https://chromiumcodereview.appspot.com/1376783004/diff/1/chrome/browser/browser_shutdown.cc File chrome/browser/browser_shutdown.cc (right): https://chromiumcodereview.appspot.com/1376783004/diff/1/chrome/browser/browser_shutdown.cc#newcode252 chrome/browser/browser_shutdown.cc:252: #elif defined(OS_LINUX) On 2015/09/30 14:39:43, gab wrote: > Should ...
5 years, 2 months ago (2015-09-30 23:33:55 UTC) #6
Lei Zhang
https://codereview.chromium.org/1376783004/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): https://codereview.chromium.org/1376783004/diff/20001/chrome/chrome_browser.gypi#oldcode654 chrome/chrome_browser.gypi:654: 'browser/memory_details.cc', Whoops, I moved too much.
5 years, 2 months ago (2015-10-01 05:41:33 UTC) #7
Lei Zhang
isherman: TBR since gab already looked at the metrics files.
5 years, 2 months ago (2015-10-01 06:25:48 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376783004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376783004/60001
5 years, 2 months ago (2015-10-01 06:26:46 UTC) #11
commit-bot: I haz the power
Committed patchset #3 (id:60001)
5 years, 2 months ago (2015-10-01 06:40:35 UTC) #12
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/6ad7fe5183699ee528f9e7961cdef3124b1144fb Cr-Commit-Position: refs/heads/master@{#351738}
5 years, 2 months ago (2015-10-01 06:41:31 UTC) #13
Ilya Sherman
https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (left): https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h#oldcode42 chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:42: #endif // defined(OS_MACOSX) && !defined(OS_IOS) Hmm, why did you ...
5 years, 2 months ago (2015-10-01 06:45:26 UTC) #14
Lei Zhang
On 2015/10/01 06:45:26, Ilya Sherman wrote: > https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (left): > > https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h#oldcode42 ...
5 years, 2 months ago (2015-10-01 06:54:10 UTC) #15
Ilya Sherman
On 2015/10/01 06:54:10, Lei Zhang wrote: > On 2015/10/01 06:45:26, Ilya Sherman wrote: > > ...
5 years, 2 months ago (2015-10-01 06:55:46 UTC) #16
Lei Zhang
On 2015/10/01 06:55:46, Ilya Sherman wrote: > On 2015/10/01 06:54:10, Lei Zhang wrote: > > ...
5 years, 2 months ago (2015-10-01 07:00:00 UTC) #17
Ilya Sherman
5 years, 2 months ago (2015-10-01 07:08:50 UTC) #18
Message was sent while issue was closed. 
On 2015/10/01 07:00:00, Lei Zhang wrote:
> On 2015/10/01 06:55:46, Ilya Sherman wrote:
> > On 2015/10/01 06:54:10, Lei Zhang wrote:
> > > On 2015/10/01 06:45:26, Ilya Sherman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/...
> > > > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h
> > (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1376783004/diff/60001/chrome/browser/metrics/...
> > > > chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:42:
> #endif 
> > > //
> > > > defined(OS_MACOSX) && !defined(OS_IOS)
> > > > Hmm, why did you remove the IOS test?
> > > 
> > > In reply to gab's comments, I found this file is not used on iOS at all.
> > 
> > I'm not sure whether that's true for the downstream code.  Might be worth
> > checking with the iOS team.  (I know that they're trying hard to upstream
and
> > stop using any //chrome code, but I'm not certain how far they are, or
whether
> > this file is used downstream.)
> 
> I checked my iOS downstream build dir earlier today for the build artifacts.
> Just now, I ran:
> 
> $ ninja -C out/Debug-iphonesimulator
> ../../chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc^
> ninja: Entering directory `out/Debug-iphonesimulator'
> ninja: error: unknown target
> '../../chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc'

Ok, great.  Thanks for double-checking =)

Powered by Google App Engine
This is Rietveld 408576698