Bindings: Require [CachedAttribute] method to be const

Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.

Update the one place (navigator.languages) where a non-const method
was being used.

Review-Url: https://codereview.chromium.org/2884493002
Cr-Commit-Position: refs/heads/master@{#472247}
Committed: https://chromium.googlesource.com/chromium/src/+/001f6234b60a15a1b009a3df5089e2adc9cde03b

[jsbell, 2017-05-15 17:38:30]

Description was changed from

==========
Bindings: Require [CachedAttribute] method to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generatd code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.
==========

to

==========
Bindings: Require [CachedAttribute] method to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.
==========

[jsbell, 2017-05-15 17:42:29]

jsbell@chromium.org changed reviewers:
+ bashi@chromium.org

[jsbell, 2017-05-15 17:42:30]

bashi@ - This is not ready to land yet (blocked on crbug.com/721933 --
running/rebasing the bindings tests) but I wanted to see what you thought.

I noticed that we were inconsistent in how we implemented CachedAttribute
callbacks when reviewing https://codereview.chromium.org/2875013002 -- I'm not
sure if we *want* to be consistent but it seems like a good idea to me.

Does this seem worthwhile?

[bashi, 2017-05-15 18:01:44]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[bashi, 2017-05-15 18:01:44]

Yes, this change totally makes sense to me. +yukishiino, haraken, WDYT?

FYI: I uploaded a fix for crbug.com/721933
(https://codereview.chromium.org/2887453003/)

[haraken1, 2017-05-15 18:19:01]

haraken@google.com changed reviewers:
+ haraken@google.com

[haraken1, 2017-05-15 18:19:02]

LGTM

(Once the lazy cached accessor is available, I'd like to replace
[CachedAttribute] with the lazy cached accessor :-)

[jsbell, 2017-05-15 21:16:50]

Description was changed from

==========
Bindings: Require [CachedAttribute] method to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.
==========

to

==========
Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.
==========

[Yuki, 2017-05-16 06:25:54]

LGTM.

[jsbell, 2017-05-16 17:56:16]

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

[commit-bot: I haz the power, 2017-05-16 17:57:41]

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-05-16 18:26:37]

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

[commit-bot: I haz the power, 2017-05-16 18:26:38]

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

[jsbell, 2017-05-16 19:02:51]

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

[commit-bot: I haz the power, 2017-05-16 19:05:24]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-05-16 19:05:25]

Description was changed from

==========
Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.
==========

to

==========
Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.

Update the one place (navigator.languages) where a non-const method
was being used.
==========

[jsbell, 2017-05-16 19:05:26]

jsbell@chromium.org changed reviewers:
+ dcheng@chromium.org

[jsbell, 2017-05-16 19:06:46]

Thanks, bindings folks!

dcheng@ - can you do an OWNERS review for Source/core/frame and note the
comment...

https://codereview.chromium.org/2884493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorLanguage.h (right):

https://codereview.chromium.org/2884493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorLanguage.h:22: protected:
I don't have a big preference between using `protected` vs. another accessor vs.
having SetLanguagesChanged() take a bool vs. ..., but this seemed to keep the
surface area small. Happy to change it.

[dcheng, 2017-05-16 21:44:10]

LGTM, but why do we even have NavigatorLanguage? It seems like it'd be easier to
just move it into Navigator (as a followup, maybe?)

[commit-bot: I haz the power, 2017-05-16 22:14:12]

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

[commit-bot: I haz the power, 2017-05-16 22:14:13]

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

[jsbell, 2017-05-16 22:18:53]

On 2017/05/16 21:44:10, dcheng (in AEST) wrote:
> why do we even have NavigatorLanguage? 

I assume it's to map to the spec, which calls out NavigatorLanguage as a
separate interface:

https://html.spec.whatwg.org/multipage/webappapis.html#navigatorlanguage

... and there's a 'TODO(foolip): WorkerNavigator implements NavigatorLanguage'
in WorkerNavigator.idl.

[jsbell, 2017-05-16 22:19:40]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-05-16 22:19:40]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@google.com,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2884493002/#ps40001
(title: "update binding test expectations")

[commit-bot: I haz the power, 2017-05-16 22:20:44]

CQ is trying da patch.

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

[dcheng, 2017-05-16 22:29:29]

On 2017/05/16 22:18:53, jsbell wrote:
> On 2017/05/16 21:44:10, dcheng (in AEST) wrote:
> > why do we even have NavigatorLanguage? 
> 
> I assume it's to map to the spec, which calls out NavigatorLanguage as a
> separate interface:
> 
> https://html.spec.whatwg.org/multipage/webappapis.html#navigatorlanguage
> 
> ... and there's a 'TODO(foolip): WorkerNavigator implements NavigatorLanguage'
> in WorkerNavigator.idl.

We don't need to have a separate C++ interface though, AFAIK. The current split
just feels a bit awkward. Either more logic should be pushed into
NavigatorLanguage::languages() or the interface should just go away. I suspect
it'll be the former if someone implements WorkerNavigator.

[commit-bot: I haz the power, 2017-05-16 23:36:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494973180189360,
"parent_rev": "87dc11bdb4e1cf4ba674f3907fa1127f8839ad40", "commit_rev":
"001f6234b60a15a1b009a3df5089e2adc9cde03b"}

[commit-bot: I haz the power, 2017-05-16 23:36:30]

Description was changed from

==========
Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.

Update the one place (navigator.languages) where a non-const method
was being used.
==========

to

==========
Bindings: Require [CachedAttribute] callbacks to be const

Since most "isAttribDirty()" methods were already const, require it
in the docs and via casting in the generated code to ensure that the
check is idempotent. Mostly for consistency, but this prevents us
from triggering unexpected side effects in future optimizations.

Update the one place (navigator.languages) where a non-const method
was being used.

Review-Url: https://codereview.chromium.org/2884493002
Cr-Commit-Position: refs/heads/master@{#472247}
Committed:
https://chromium.googlesource.com/chromium/src/+/001f6234b60a15a1b009a3df5089...
==========

[commit-bot: I haz the power, 2017-05-16 23:36:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/001f6234b60a15a1b009a3df5089...