Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827272:
Drive: Hide the URL on opening files on Drive. (Closed)
|
Created:
8 years, 4 months ago by yoshiki Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDrive: Hide the URL on opening files on Drive.
BUG=136394
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151158
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review (#2) fix #
Total comments: 1
Messages
Total messages: 14 (0 generated)
Aaron, could you take a look? You've written the code around here.
http://codereview.chromium.org/10827272/diff/1/chrome/browser/ui/toolbar/tool... File chrome/browser/ui/toolbar/toolbar_model.cc (right): http://codereview.chromium.org/10827272/diff/1/chrome/browser/ui/toolbar/tool... chrome/browser/ui/toolbar/toolbar_model.cc:91: #if defined(OS_CHROMEOS) I'm not a fan of mixing runtime and compile time conditionals this way. I'd prefer just having a separate if statement that was entirely guarded by OS_CHROMEOS.
Also, I can't actually approve changes in this directory.
+sky Sky, could you take a look as a owner? Aaron, PTAL. http://codereview.chromium.org/10827272/diff/1/chrome/browser/ui/toolbar/tool... File chrome/browser/ui/toolbar/toolbar_model.cc (right): http://codereview.chromium.org/10827272/diff/1/chrome/browser/ui/toolbar/tool... chrome/browser/ui/toolbar/toolbar_model.cc:91: #if defined(OS_CHROMEOS) On 2012/08/10 04:36:56, Aaron Boodman wrote: > I'm not a fan of mixing runtime and compile time conditionals this way. I'd > prefer just having a separate if statement that was entirely guarded by > OS_CHROMEOS. Done.
+sky Sky, could you take a look as a owner? Aaron, PTAL.
+sky Sky, could you take a look as a owner? Aaron, PTAL.
http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... File chrome/browser/ui/toolbar/toolbar_model.cc (right): http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... chrome/browser/ui/toolbar/toolbar_model.cc:94: if (entry && entry->GetURL().SchemeIs(chrome::kDriveScheme)) Expose this by way of a value in chrome/browser/defaults.h that is only set true for chromeos. That way, no ifdef here.
We need ifdef, because no ifdef causes a compile error. chrome::kDriveScheme is decrared only on chromeos. On 2012/08/10 16:04:10, sky wrote: > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... > File chrome/browser/ui/toolbar/toolbar_model.cc (right): > > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... > chrome/browser/ui/toolbar/toolbar_model.cc:94: if (entry && > entry->GetURL().SchemeIs(chrome::kDriveScheme)) > Expose this by way of a value in chrome/browser/defaults.h that is only set true > for chromeos. That way, no ifdef here.
There will still be an ifdef, but in defaults.cc and not toolbar_model. -Scott On Fri, Aug 10, 2012 at 10:29 AM, <yoshiki@chromium.org> wrote: > We need ifdef, because no ifdef causes a compile error. chrome::kDriveScheme > is > decrared only on chromeos. > > > On 2012/08/10 16:04:10, sky wrote: > > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... >> >> File chrome/browser/ui/toolbar/toolbar_model.cc (right): > > > > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... >> >> chrome/browser/ui/toolbar/toolbar_model.cc:94: if (entry && >> entry->GetURL().SchemeIs(chrome::kDriveScheme)) >> Expose this by way of a value in chrome/browser/defaults.h that is only >> set > > true >> >> for chromeos. That way, no ifdef here. > > > > http://codereview.chromium.org/10827272/
Sorry, I'm confusing. I can't get "way of a value in chrome/browser/defaults.h". Could you tell me what do you think? I think, we can't remove ifdef from toolbar_model unless we stop to use "chrome::kDriveScheme" in toolbar_model. This chrome::kDriveScheme is declared only on chromeos in chrome/common/url_constants.h. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/url_constant... On 2012/08/10 19:06:26, sky wrote: > There will still be an ifdef, but in defaults.cc and not toolbar_model. > > -Scott > > On Fri, Aug 10, 2012 at 10:29 AM, <mailto:yoshiki@chromium.org> wrote: > > We need ifdef, because no ifdef causes a compile error. chrome::kDriveScheme > > is > > decrared only on chromeos. > > > > > > On 2012/08/10 16:04:10, sky wrote: > > > > > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... > >> > >> File chrome/browser/ui/toolbar/toolbar_model.cc (right): > > > > > > > > > http://codereview.chromium.org/10827272/diff/3/chrome/browser/ui/toolbar/tool... > >> > >> chrome/browser/ui/toolbar/toolbar_model.cc:94: if (entry && > >> entry->GetURL().SchemeIs(chrome::kDriveScheme)) > >> Expose this by way of a value in chrome/browser/defaults.h that is only > >> set > > > > true > >> > >> for chromeos. That way, no ifdef here. > > > > > > > > http://codereview.chromium.org/10827272/
My mistake, I see what you're saying. LGTM
On 2012/08/10 22:23:46, sky wrote: > My mistake, I see what you're saying. LGTM Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10827272/3
Change committed as 151158 |
