Disable the image notification property on MacOS

Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests

TBR=avi
BUG=571056
Review-Url: https://codereview.chromium.org/2814553002
Cr-Commit-Position: refs/heads/master@{#464492}
Committed: https://chromium.googlesource.com/chromium/src/+/65fa642573afc775711271b1ab8ef1b24c07bd6e

[Miguel Garcia, 2017-04-10 18:10:07]

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

[commit-bot: I haz the power, 2017-04-10 18:10:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 18:27:30]

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

[commit-bot: I haz the power, 2017-04-10 18:27:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miguel Garcia, 2017-04-10 18:47:53]

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

[commit-bot: I haz the power, 2017-04-10 18:48:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 20:35:30]

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

[commit-bot: I haz the power, 2017-04-10 20:35:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2017-04-10 23:23:21]

This will work :) There's still a bunch of expectation files to modify, but
there should be less.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/platform/...

We'd probably need to fork the worker ones too, since we (unlike many other
APIs) are available there too.

[Peter Beverloo, 2017-04-10 23:23:41]

Description was changed from

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests


BUG=571056
==========

to

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests


BUG=571056
==========

[Peter Beverloo, 2017-04-10 23:23:41]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Miguel Garcia, 2017-04-11 13:05:13]

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

[commit-bot: I haz the power, 2017-04-11 13:05:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-11 15:51:52]

PTAL I added the missing platform specific files.

On 2017/04/10 23:23:21, Peter Beverloo wrote:
> This will work :) There's still a bunch of expectation files to modify, but
> there should be less.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/platform/...
> 
> We'd probably need to fork the worker ones too, since we (unlike many other
> APIs) are available there too.

[commit-bot: I haz the power, 2017-04-11 16:43:36]

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

[commit-bot: I haz the power, 2017-04-11 16:43:37]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-04-11 17:02:05]

lgtm

[Miguel Garcia, 2017-04-11 19:15:42]

miguelg@chromium.org changed reviewers:
+ avi@chromium.org, rbyers@chromium.org

[Miguel Garcia, 2017-04-11 19:15:43]

Avi for OWNERS (this is roughly a revert of 
https://codereview.chromium.org/2806233003

rbyers@ for the change in the API file.

 Note that I tried the more naive way but the number of platform files I had to
add did not really make it worth it.

[Miguel Garcia, 2017-04-12 07:05:52]

Rick can you have a look? I would like to get this in before branch point if
possible.


On 2017/04/11 19:15:43, Miguel Garcia wrote:
> Avi for OWNERS (this is roughly a revert of 
> https://codereview.chromium.org/2806233003
> 
> rbyers@ for the change in the API file.
> 
>  Note that I tried the more naive way but the number of platform files I had
to
> add did not really make it worth it.

[Miguel Garcia, 2017-04-13 08:33:35]

miguelg@chromium.org changed reviewers:
+ foolip@chromium.org

[Miguel Garcia, 2017-04-13 08:33:36]

+foolip as are approaching branch point and I'd like to have this in.

[foolip, 2017-04-13 08:38:12]

lgtm % changing the default status to "stable"

https://codereview.chromium.org/2814553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2814553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:657: status:
"test",
Can you keep status "stable" and disable it on Mac instead? When trying to
figure out which parts of our IDL we are actually shipping, i.e. where it's
important that it matches the spec, it's a problem if it looks like a feature
isn't shipping but actually is. There are some cases, but the fewer the better.

[Miguel Garcia, 2017-04-13 09:08:47]

That was my initial intention (see https://codereview.chromium.
org/2803303002/) but if I do that I have to create per platform files for
many other tests which seems like a huge burden for everybody just to
disable the image property on mac. This is what other APIs in this
situation have done (except in this case it would be much worse because
notifications is available in workers).

We should probably discuss having some kind of annotation in the
expectation files since this is not scalable but with the tools I have
Today this way seemed like the least harmful approach.

On Thu, Apr 13, 2017 at 9:38 AM, <foolip@chromium.org> wrote:

> lgtm % changing the default status to "stable"
>
>
> https://codereview.chromium.org/2814553002/diff/40001/
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> (right):
>
> https://codereview.chromium.org/2814553002/diff/40001/
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode657
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:657:
> status: "test",
> Can you keep status "stable" and disable it on Mac instead? When trying
> to figure out which parts of our IDL we are actually shipping, i.e.
> where it's important that it matches the spec, it's a problem if it
> looks like a feature isn't shipping but actually is. There are some
> cases, but the fewer the better.
>
> https://codereview.chromium.org/2814553002/
>

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

[Miguel Garcia, 2017-04-13 09:08:48]

That was my initial intention (see https://codereview.chromium.
org/2803303002/) but if I do that I have to create per platform files for
many other tests which seems like a huge burden for everybody just to
disable the image property on mac. This is what other APIs in this
situation have done (except in this case it would be much worse because
notifications is available in workers).

We should probably discuss having some kind of annotation in the
expectation files since this is not scalable but with the tools I have
Today this way seemed like the least harmful approach.

On Thu, Apr 13, 2017 at 9:38 AM, <foolip@chromium.org> wrote:

> lgtm % changing the default status to "stable"
>
>
> https://codereview.chromium.org/2814553002/diff/40001/
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> (right):
>
> https://codereview.chromium.org/2814553002/diff/40001/
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode657
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:657:
> status: "test",
> Can you keep status "stable" and disable it on Mac instead? When trying
> to figure out which parts of our IDL we are actually shipping, i.e.
> where it's important that it matches the spec, it's a problem if it
> looks like a feature isn't shipping but actually is. There are some
> cases, but the fewer the better.
>
> https://codereview.chromium.org/2814553002/
>

-- 
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.

[foolip, 2017-04-13 09:37:51]

On 2017/04/13 09:08:48, Miguel Garcia wrote:
> On Thu, Apr 13, 2017 at 9:38 AM, <mailto:foolip@chromium.org> wrote:
> 
> > lgtm % changing the default status to "stable"
>
> That was my initial intention (see https://codereview.chromium.
> org/2803303002/) but if I do that I have to create per platform files for
> many other tests which seems like a huge burden for everybody just to
> disable the image property on mac. This is what other APIs in this
> situation have done (except in this case it would be much worse because
> notifications is available in workers).
> 
> We should probably discuss having some kind of annotation in the
> expectation files since this is not scalable but with the tools I have
> Today this way seemed like the least harmful approach.

Hmm, OK then, we'll have to figure out a solution for this eventually, but not
today then.

[Miguel Garcia, 2017-04-13 11:45:28]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-04-13 11:45:37]

The CQ bit was unchecked by miguelg@chromium.org

[commit-bot: I haz the power, 2017-04-13 11:45:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-13 11:45:50]

Description was changed from

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests


BUG=571056
==========

to

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests

TBR=avi
BUG=571056
==========

[Miguel Garcia, 2017-04-13 11:46:43]

On 2017/04/13 09:37:51, foolip_UTC7 wrote:
> On 2017/04/13 09:08:48, Miguel Garcia wrote:
> > On Thu, Apr 13, 2017 at 9:38 AM, <mailto:foolip@chromium.org> wrote:
> > 
> > > lgtm % changing the default status to "stable"
> >
> > That was my initial intention (see https://codereview.chromium.
> > org/2803303002/) but if I do that I have to create per platform files for
> > many other tests which seems like a huge burden for everybody just to
> > disable the image property on mac. This is what other APIs in this
> > situation have done (except in this case it would be much worse because
> > notifications is available in workers).
> > 
> > We should probably discuss having some kind of annotation in the
> > expectation files since this is not scalable but with the tools I have
> > Today this way seemed like the least harmful approach.
> 
> Hmm, OK then, we'll have to figure out a solution for this eventually, but not
> today then.

Thanks!!

I am TBRing avi since he is OOO and the files he approved in my previews attempt
have not changed.

[Miguel Garcia, 2017-04-13 11:46:51]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2017-04-13 11:46:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 11:55:04]

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

[commit-bot: I haz the power, 2017-04-13 11:55:06]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Miguel Garcia, 2017-04-13 12:39:16]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2017-04-13 12:39:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-04-13 14:21:31]

lgtm

stamp

[Rick Byers, 2017-04-13 14:25:16]

Sorry for the delay.  I had a reply prepared yesterday but wanted to diff the
expected output files before sending and got pulled into a firedill.   Anyway
retroactive LGTM.

On 2017/04/13 09:08:48, Miguel Garcia wrote:
> That was my initial intention (see https://codereview.chromium.
> org/2803303002/) but if I do that I have to create per platform files for
> many other tests which seems like a huge burden for everybody just to
> disable the image property on mac. This is what other APIs in this
> situation have done (except in this case it would be much worse because
> notifications is available in workers).

Are you talking about tests other than the webexposed tests here?  In principle
enabling by default and disabling on Mac should be exactly equivalent to
disabling by default and re-enabling on Mac.  How exactly are the test
expectations any different between those two scenarios?  The above CL doesn't
have any changed expectations files, so the problem isn't obvious there.
 
> We should probably discuss having some kind of annotation in the
> expectation files since this is not scalable but with the tools I have
> Today this way seemed like the least harmful approach.

Yeah we've discussed this before and I had proposed a simple solution.  I think
it's time to prioritize that.  Filed https://crbug.com/711292

Also related, filed https://crbug.com/711300

> On Thu, Apr 13, 2017 at 9:38 AM, <mailto:foolip@chromium.org> wrote:
> 
> > lgtm % changing the default status to "stable"
> >
> >
> > https://codereview.chromium.org/2814553002/diff/40001/
> > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> > (right):
> >
> > https://codereview.chromium.org/2814553002/diff/40001/
> > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode657
> > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:657:
> > status: "test",
> > Can you keep status "stable" and disable it on Mac instead? When trying
> > to figure out which parts of our IDL we are actually shipping, i.e.
> > where it's important that it matches the spec, it's a problem if it
> > looks like a feature isn't shipping but actually is. There are some
> > cases, but the fewer the better.
> >
> > https://codereview.chromium.org/2814553002/
> >
> 
> -- 
> 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.

[commit-bot: I haz the power, 2017-04-13 14:39:18]

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

[commit-bot: I haz the power, 2017-04-13 14:39:19]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Miguel Garcia, 2017-04-13 17:20:18]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-04-13 17:20:18]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, rbyers@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2814553002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-13 17:20:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 19:11:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492104018226400,
"parent_rev": "844858e34bd823d24ee1ac41b23203c7fb597d31", "commit_rev":
"65fa642573afc775711271b1ab8ef1b24c07bd6e"}

[commit-bot: I haz the power, 2017-04-13 19:12:22]

Description was changed from

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests

TBR=avi
BUG=571056
==========

to

==========
Disable the image notification property on MacOS

This is in preparation for the launch of MacOSX native notifications
where the property is not supported.

Intent to remove
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IKGgHJOVMYE

A more naive attempt was attempted in
https://codereview.chromium.org/2803303002/
and reverted because of it broke several tests

TBR=avi
BUG=571056

Review-Url: https://codereview.chromium.org/2814553002
Cr-Commit-Position: refs/heads/master@{#464492}
Committed:
https://chromium.googlesource.com/chromium/src/+/65fa642573afc775711271b1ab8e...
==========

[commit-bot: I haz the power, 2017-04-13 19:12:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/65fa642573afc775711271b1ab8e...

[Miguel Garcia, 2017-04-16 06:50:49]

On 2017/04/13 14:25:16, Rick Byers wrote:
> Sorry for the delay.  I had a reply prepared yesterday but wanted to diff the
> expected output files before sending and got pulled into a firedill.   Anyway
> retroactive LGTM.
> 
> On 2017/04/13 09:08:48, Miguel Garcia wrote:
> > That was my initial intention (see https://codereview.chromium.
> > org/2803303002/) but if I do that I have to create per platform files for
> > many other tests which seems like a huge burden for everybody just to
> > disable the image property on mac. This is what other APIs in this
> > situation have done (except in this case it would be much worse because
> > notifications is available in workers).
> 
> Are you talking about tests other than the webexposed tests here?  In
principle
> enabling by default and disabling on Mac should be exactly equivalent to
> disabling by default and re-enabling on Mac.  How exactly are the test
> expectations any different between those two scenarios?  The above CL doesn't
> have any changed expectations files, so the problem isn't obvious there.

Yes, looking at the way I phrased it was not obvious, I think we can get an idea
of the tests that fail in either case by looking at the failed try bots for CL
in my first attempt

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...

And compare that with the tests that needed to be fixed with this CL
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...



>  
> > We should probably discuss having some kind of annotation in the
> > expectation files since this is not scalable but with the tools I have
> > Today this way seemed like the least harmful approach.
> 
> Yeah we've discussed this before and I had proposed a simple solution.  I
think
> it's time to prioritize that.  Filed https://crbug.com/711292
> 
> Also related, filed https://crbug.com/711300
> 
> > On Thu, Apr 13, 2017 at 9:38 AM, <mailto:foolip@chromium.org> wrote:
> > 
> > > lgtm % changing the default status to "stable"
> > >
> > >
> > > https://codereview.chromium.org/2814553002/diff/40001/
> > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> > > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
> > > (right):
> > >
> > > https://codereview.chromium.org/2814553002/diff/40001/
> > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode657
> > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:657:
> > > status: "test",
> > > Can you keep status "stable" and disable it on Mac instead? When trying
> > > to figure out which parts of our IDL we are actually shipping, i.e.
> > > where it's important that it matches the spec, it's a problem if it
> > > looks like a feature isn't shipping but actually is. There are some
> > > cases, but the fewer the better.
> > >
> > > https://codereview.chromium.org/2814553002/
> > >
> > 
> > -- 
> > 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.