Remove kDisableDownloadNotification and stop compiling shelf code on

Remove kDisableDownloadNotification and stop compiling shelf code on
CrOS.

BUG=587273, 217810, 582688

[Evan Stade, 2016-02-18 02:40:24]

Description was changed from

==========
Remove kDisableDownloadNotification and stop compiling shelf code on
CrOS.

BUG=587273, 217810
==========

to

==========
Remove kDisableDownloadNotification and stop compiling shelf code on
CrOS.

BUG=587273, 217810, 582688
==========

[Evan Stade, 2016-02-18 02:40:25]

estade@chromium.org changed reviewers:
+ asanka@chromium.org, sky@chromium.org

[Evan Stade, 2016-02-18 02:57:03]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 02:57:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704793002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704793002/100001

[commit-bot: I haz the power, 2016-02-18 03:00:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 03:00:59]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[sky, 2016-02-18 22:24:10]

I have to admit I'm not a fan of the slew of random ifdefs this adds. I would
prefer we continue down the path of using a feature to determine if
downloadshelf is available, and having chromeos return false for it.

[Evan Stade, 2016-02-18 22:31:50]

On 2016/02/18 22:24:10, sky wrote:
> I have to admit I'm not a fan of the slew of random ifdefs this adds. I would
> prefer we continue down the path of using a feature to determine if
> downloadshelf is available, and having chromeos return false for it.

Can you expand on why you dislike ifdefs? I agree fewer are better than more,
but I disagree that a runtime check is better. Perhaps you'll need fewer runtime
checks, but to me the difference is this:

#if !defined(OS_CHROMEOS)
void DoSomething() {
  ...
}
#endif

vs

// It's impossible to tell from here, but this function is never used on certain
platforms.
void DoSomething() {
  ...
}

When it comes to readability, ifdefs can add clutter, but at least the former is
explicit about what's actually happening.

I think ideally we would compile out the code and use fewer ifdefs. The only
reason we need so many is because of what (imo) amounts to a bunch of layering
violations. But I think the objective benefits of code removal outweigh the
aesthetic concerns over ifdefs (unless there is some reason to dislike ifdefs
that I haven't considered).

[sky, 2016-02-19 00:27:42]

All the ifdefs in browser_view make me sad.

On Thu, Feb 18, 2016 at 2:31 PM,  <estade@chromium.org> wrote:
> On 2016/02/18 22:24:10, sky wrote:
>> I have to admit I'm not a fan of the slew of random ifdefs this adds. I
>> would
>> prefer we continue down the path of using a feature to determine if
>> downloadshelf is available, and having chromeos return false for it.
>
> Can you expand on why you dislike ifdefs? I agree fewer are better than
> more,
> but I disagree that a runtime check is better. Perhaps you'll need fewer
> runtime
> checks, but to me the difference is this:
>
> #if !defined(OS_CHROMEOS)
> void DoSomething() {
>   ...
> }
> #endif
>
> vs
>
> // It's impossible to tell from here, but this function is never used on
> certain
> platforms.
> void DoSomething() {
>   ...
> }
>
> When it comes to readability, ifdefs can add clutter, but at least the
> former is
> explicit about what's actually happening.
>
> I think ideally we would compile out the code and use fewer ifdefs. The only
> reason we need so many is because of what (imo) amounts to a bunch of
> layering
> violations. But I think the objective benefits of code removal outweigh the
> aesthetic concerns over ifdefs (unless there is some reason to dislike
> ifdefs
> that I haven't considered).
>
> https://codereview.chromium.org/1704793002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-02-23 18:10:17]

I've been meaning to come back to this to see if I could improve it but probably
won't get to it till after the branch point. Would you like it better if I added
a feature BUILDFLAG like in [1]?

[1] https://codereview.chromium.org/1718093005/

On 2016/02/19 00:27:42, sky wrote:
> All the ifdefs in browser_view make me sad.
> 
> On Thu, Feb 18, 2016 at 2:31 PM,  <mailto:estade@chromium.org> wrote:
> > On 2016/02/18 22:24:10, sky wrote:
> >> I have to admit I'm not a fan of the slew of random ifdefs this adds. I
> >> would
> >> prefer we continue down the path of using a feature to determine if
> >> downloadshelf is available, and having chromeos return false for it.
> >
> > Can you expand on why you dislike ifdefs? I agree fewer are better than
> > more,
> > but I disagree that a runtime check is better. Perhaps you'll need fewer
> > runtime
> > checks, but to me the difference is this:
> >
> > #if !defined(OS_CHROMEOS)
> > void DoSomething() {
> >   ...
> > }
> > #endif
> >
> > vs
> >
> > // It's impossible to tell from here, but this function is never used on
> > certain
> > platforms.
> > void DoSomething() {
> >   ...
> > }
> >
> > When it comes to readability, ifdefs can add clutter, but at least the
> > former is
> > explicit about what's actually happening.
> >
> > I think ideally we would compile out the code and use fewer ifdefs. The only
> > reason we need so many is because of what (imo) amounts to a bunch of
> > layering
> > violations. But I think the objective benefits of code removal outweigh the
> > aesthetic concerns over ifdefs (unless there is some reason to dislike
> > ifdefs
> > that I haven't considered).
> >
> > https://codereview.chromium.org/1704793002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-02-23 21:21:11]

It's still going to be a slew of ifdefs, right? So, probably not.

  -Scott

On Tue, Feb 23, 2016 at 10:10 AM,  <estade@chromium.org> wrote:
> I've been meaning to come back to this to see if I could improve it but
> probably
> won't get to it till after the branch point. Would you like it better if I
> added
> a feature BUILDFLAG like in [1]?
>
> [1] https://codereview.chromium.org/1718093005/
>
> On 2016/02/19 00:27:42, sky wrote:
>> All the ifdefs in browser_view make me sad.
>>
>> On Thu, Feb 18, 2016 at 2:31 PM, <mailto:estade@chromium.org> wrote:
>> > On 2016/02/18 22:24:10, sky wrote:
>> >> I have to admit I'm not a fan of the slew of random ifdefs this adds. I
>> >> would
>> >> prefer we continue down the path of using a feature to determine if
>> >> downloadshelf is available, and having chromeos return false for it.
>> >
>> > Can you expand on why you dislike ifdefs? I agree fewer are better than
>> > more,
>> > but I disagree that a runtime check is better. Perhaps you'll need fewer
>> > runtime
>> > checks, but to me the difference is this:
>> >
>> > #if !defined(OS_CHROMEOS)
>> > void DoSomething() {
>> > ...
>> > }
>> > #endif
>> >
>> > vs
>> >
>> > // It's impossible to tell from here, but this function is never used on
>> > certain
>> > platforms.
>> > void DoSomething() {
>> > ...
>> > }
>> >
>> > When it comes to readability, ifdefs can add clutter, but at least the
>> > former is
>> > explicit about what's actually happening.
>> >
>> > I think ideally we would compile out the code and use fewer ifdefs. The
>> > only
>> > reason we need so many is because of what (imo) amounts to a bunch of
>> > layering
>> > violations. But I think the objective benefits of code removal outweigh
>> > the
>> > aesthetic concerns over ifdefs (unless there is some reason to dislike
>> > ifdefs
>> > that I haven't considered).
>> >
>> > https://codereview.chromium.org/1704793002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1704793002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.