[Experimental Framework] Only log a console message for each feature once

[Experimental Framework] Only log a console message for each feature once

BUG=584380
Committed: https://crrev.com/d83168f09a52a990cea93ab20264f431a3adcbc8
Cr-Commit-Position: refs/heads/master@{#379656}

[chasej, 2016-02-26 21:15:34]

Description was changed from

==========
[Experimental Framework] Only log a console message for each feature once

BUG=
==========

to

==========
[Experimental Framework] Only log a console message for each feature once

BUG=584380
==========

[iclelland, 2016-02-29 18:22:58]

iclelland@chromium.org changed reviewers:
+ chasej@chromium.org

[iclelland, 2016-02-29 18:22:59]

+r chasej, PTAL, thanks!

[chasej, 2016-02-29 19:08:53]

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:88: }
On subsequent calls, this implementation leaves the error message variable
unchanged (after the first call for a disabled feature).  Maybe the message
should explicitly be set to empty? If the caller reuses a variable in subsequent
calls, the message could appear to be set.  In other words, it might be better
to have more deterministic behaviour, where the errorMessage will always be set
when the method returns false (to some text on first call, and empty on all
following calls).

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:55: // is
required (for testing, for instance).
I'd suggest adding an explanation for how the errorMessage is expected to be
used (i.e. only provided on first call for a disabled feature). This is indirect
control over the desired behaviour (i.e. causing console logs to appear), so I'd
say some comment is justified.

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:62:
HashSet<String> m_consoleLogEmitted;
This is really tracking whether an error message has been generated/emitted. I
don't think this should be described in terms of console logs, since that
behaviour is controlled elsewhere. The comment and member name should be updated
appropriately.

[iclelland, 2016-02-29 20:26:33]

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:88: }
On 2016/02/29 19:08:53, chasej wrote:
> On subsequent calls, this implementation leaves the error message variable
> unchanged (after the first call for a disabled feature).  Maybe the message
> should explicitly be set to empty? If the caller reuses a variable in
subsequent
> calls, the message could appear to be set.  In other words, it might be better
> to have more deterministic behaviour, where the errorMessage will always be
set
> when the method returns false (to some text on first call, and empty on all
> following calls).

Done.

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:55: // is
required (for testing, for instance).
On 2016/02/29 19:08:53, chasej wrote:
> I'd suggest adding an explanation for how the errorMessage is expected to be
> used (i.e. only provided on first call for a disabled feature). This is
indirect
> control over the desired behaviour (i.e. causing console logs to appear), so
I'd
> say some comment is justified.

Done. I added a comment in the header where the member is defined, and at the
point where we clear the error string as well.

https://codereview.chromium.org/1739163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:62:
HashSet<String> m_consoleLogEmitted;
On 2016/02/29 19:08:53, chasej wrote:
> This is really tracking whether an error message has been generated/emitted. I
> don't think this should be described in terms of console logs, since that
> behaviour is controlled elsewhere. The comment and member name should be
updated
> appropriately.

Done.

[chasej, 2016-03-01 16:01:13]

LGTM, with nits.

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:91: // with
error messages on every attempts to access the feature.)
Nit: should be "attempt", singular not plural.

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(right):

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:201:
TEST_F(OriginTrialContextTest, OnlyOneConsoleMessageLogged)
Nit: This isn't really testing if console messages are logged. Ideally, the test
would be renamed, similar to how the internal implementation was changed.

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:222:
TEST_F(OriginTrialContextTest, ConsoleMessageLoggedPerFeature)
Nit: Same as above with respect to naming.

[iclelland, 2016-03-03 16:04:53]

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:91: // with
error messages on every attempts to access the feature.)
On 2016/03/01 16:01:13, chasej wrote:
> Nit: should be "attempt", singular not plural.

Done.

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(right):

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:201:
TEST_F(OriginTrialContextTest, OnlyOneConsoleMessageLogged)
On 2016/03/01 16:01:13, chasej wrote:
> Nit: This isn't really testing if console messages are logged. Ideally, the
test
> would be renamed, similar to how the internal implementation was changed.

Done.

https://codereview.chromium.org/1739163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:222:
TEST_F(OriginTrialContextTest, ConsoleMessageLoggedPerFeature)
On 2016/03/01 16:01:13, chasej wrote:
> Nit: Same as above with respect to naming.

Done.

[iclelland, 2016-03-07 17:33:50]

iclelland@chromium.org changed reviewers:
+ rbyers@chromium.org

[iclelland, 2016-03-07 17:33:50]

+r rbyers -- can you PTAL as owner?

[Rick Byers, 2016-03-07 21:28:16]

LGTM

Remind me, where exactly is the code that generates the console messages?  I
can't seem to find it.

I assume this design means that we'll generate one message per iframe, right? 
Eg. if some site happens to keep removing and recreating an iframe that tests an
experimental feature (perhaps just by naively enumerating all properties on the
'navigator' object or something) then we'll still get a continuous stream of
console warnings, right?

The alternative is to track the state per-FrameHost somehow (like the
Deprecation class does).

[iclelland, 2016-03-07 21:49:05]

On 2016/03/07 21:28:16, Rick Byers wrote:
> LGTM
> 
> Remind me, where exactly is the code that generates the console messages?  I
> can't seem to find it.

These messages are generated here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

And added to the JS console, by bindings code here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(You can see an example of the generated bindings code, for webbluetooth, here:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
)

> 
> I assume this design means that we'll generate one message per iframe, right? 
> Eg. if some site happens to keep removing and recreating an iframe that tests
an
> experimental feature (perhaps just by naively enumerating all properties on
the
> 'navigator' object or something) then we'll still get a continuous stream of
> console warnings, right?
> 
> The alternative is to track the state per-FrameHost somehow (like the
> Deprecation class does).

That's something that I will look at -- it would make sense to eventually have
them using consistent heuristics. This will be more important for usage logging,
but your point about iframes makes sense.

[iclelland, 2016-03-07 21:49:36]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-03-07 21:49:37]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/1739163004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-07 21:50:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1739163004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1739163004/120001

[commit-bot: I haz the power, 2016-03-07 21:56:27]

Description was changed from

==========
[Experimental Framework] Only log a console message for each feature once

BUG=584380
==========

to

==========
[Experimental Framework] Only log a console message for each feature once

BUG=584380
==========

[commit-bot: I haz the power, 2016-03-07 21:56:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-07 21:58:42]

Description was changed from

==========
[Experimental Framework] Only log a console message for each feature once

BUG=584380
==========

to

==========
[Experimental Framework] Only log a console message for each feature once

BUG=584380

Committed: https://crrev.com/d83168f09a52a990cea93ab20264f431a3adcbc8
Cr-Commit-Position: refs/heads/master@{#379656}
==========

[commit-bot: I haz the power, 2016-03-07 21:58:43]

Patchset 7 (id:??) landed as
https://crrev.com/d83168f09a52a990cea93ab20264f431a3adcbc8
Cr-Commit-Position: refs/heads/master@{#379656}