Absolute Device Orientation API: blink implementation.

Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add and update layout tests and UseCounters.
The 'ondeviceorientationabsolute' event listener is currently
exposed behind the experimental features flag.

BUG=520546
Committed: https://crrev.com/70e4cfe9b1a1ed76535b5b846dccef9033418313
Cr-Commit-Position: refs/heads/master@{#357374}

[timvolodine, 2015-10-21 18:19:59]

Description was changed from

==========
Absolute Device Orientation: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.

TODO: Add layout tests once crrev.com/1402923004 lands.
TODO: needs rebase

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.

TODO: Add layout tests once crrev.com/1402923004 lands.
TODO: needs rebase

BUG=520546
==========

[timvolodine, 2015-10-21 18:34:51]

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

[timvolodine, 2015-10-21 18:34:51]

rbyers@: initial patch for the blink side, ptal

[timvolodine, 2015-10-21 18:35:52]

timvolodine@chromium.org changed reviewers:
+ smus@chromium.org

[timvolodine, 2015-10-21 18:35:53]

+smus@: I've currently named the event 'deviceorientationabsolute' while we used
'absolutedeviceorientation' in the spec. From programmatic viewpoint naming
classes and constants 'DeviceOrientationAbsolute' everywhere makes sense as it
keeps them together wrt alphabetical order (which we tend to use). So what
should we name the event 'absolutedeviceorientation' or
'deviceorientationabsolute'. wdyt?

[Boris Smus, 2015-10-21 18:51:19]

SGTM. I have no naming preference here.

On Wed, Oct 21, 2015 at 11:35 AM, <timvolodine@chromium.org> wrote:

> +smus@: I've currently named the event 'deviceorientationabsolute' while
> we used
> 'absolutedeviceorientation' in the spec. From programmatic viewpoint naming
> classes and constants 'DeviceOrientationAbsolute' everywhere makes sense
> as it
> keeps them together wrt alphabetical order (which we tend to use). So what
> should we name the event 'absolutedeviceorientation' or
> 'deviceorientationabsolute'. wdyt?
>
>
> https://codereview.chromium.org/1416123002/
>



-- 
Ask your next question on Stack Overflow
<http://stackoverflow.com/questions/ask>, using the google-chrome-extension
<http://stackoverflow.com/questions/tagged/google-chrome-extension> tag.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Boris Smus, 2015-10-21 18:51:21]

SGTM. I have no naming preference here.

On Wed, Oct 21, 2015 at 11:35 AM, <timvolodine@chromium.org> wrote:

> +smus@: I've currently named the event 'deviceorientationabsolute' while
> we used
> 'absolutedeviceorientation' in the spec. From programmatic viewpoint naming
> classes and constants 'DeviceOrientationAbsolute' everywhere makes sense
> as it
> keeps them together wrt alphabetical order (which we tend to use). So what
> should we name the event 'absolutedeviceorientation' or
> 'deviceorientationabsolute'. wdyt?
>
>
> https://codereview.chromium.org/1416123002/
>



-- 
Ask your next question on Stack Overflow
<http://stackoverflow.com/questions/ask>, using the google-chrome-extension
<http://stackoverflow.com/questions/tagged/google-chrome-extension> tag.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Rick Byers, 2015-10-21 18:54:46]

Since there isn't yet an approved LGTM to ship, please put the new API behind a
RuntimeEnabledFeature (with status=experimental).  Alternately (if that's a pain
for some reason) we could block landing this on the intent-to-ship thread (but
it's usually easier to be able to point to landed code in those discussions).

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
This file seems like a fairly complete copy/paste of
DeviceOrientationController.cpp.  Can't we share the logic somehow?  Eg. make
DeviceOrientationController take a boolean parameter indicating whether it's
absolute or not and then keep two instances with the two different values?

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Ditto on code duplication from DeviceOrientationDispatcher.cpp

[Rick Byers, 2015-10-21 19:00:31]

On 2015/10/21 18:54:46, Rick Byers wrote:
> Since there isn't yet an approved LGTM to ship, please put the new API behind
a
> RuntimeEnabledFeature (with status=experimental).  Alternately (if that's a
pain
> for some reason) we could block landing this on the intent-to-ship thread (but
> it's usually easier to be able to point to landed code in those discussions).
> 
>
https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
> (right):
> 
>
https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:1:
> // Copyright 2015 The Chromium Authors. All rights reserved.
> This file seems like a fairly complete copy/paste of
> DeviceOrientationController.cpp.  Can't we share the logic somehow?  Eg. make
> DeviceOrientationController take a boolean parameter indicating whether it's
> absolute or not and then keep two instances with the two different values?
>

Actually, since this is a Document supplement I guess it's not that simple to
just have two instances of it.  But perhaps a single instance can handle both
the absolute and non-absolute events?  Or I guess you could use a template type
with two typedefs (but a template feels like it should be overkill for such a
simple use case). 

>
https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp
> (right):
> 
>
https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp:1:
> // Copyright 2015 The Chromium Authors. All rights reserved.
> Ditto on code duplication from DeviceOrientationDispatcher.cpp

[timvolodine, 2015-10-22 14:54:39]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.

TODO: Add layout tests once crrev.com/1402923004 lands.
TODO: needs rebase

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.

TODO: Add layout tests once crrev.com/1402923004 lands.

BUG=520546
==========

[timvolodine, 2015-10-22 15:06:28]

Thanks for the initial review!

I've now put the feature behind DeviceOrientationAbsolute blink flag (as
experimental). Also refactored the clasess somewhat which gets rid of about 100
lines of code. PTAL.

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/10/21 18:54:46, Rick Byers wrote:
> This file seems like a fairly complete copy/paste of
> DeviceOrientationController.cpp.  Can't we share the logic somehow?  Eg. make
> DeviceOrientationController take a boolean parameter indicating whether it's
> absolute or not and then keep two instances with the two different values?

right, ok I've refactored the code a bit to reuse the common bits. I ended up
subclassing DeviceOrientationController and templating the
DeviceOrientationDispatcher. Introducing a 'boolean' switch in controller seems
a bit hard since it uses ::from(Document) as you already mentioned. Subclassing
seems to resolve this problem by overriding the few different methods.

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteDispatcher.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/10/21 18:54:46, Rick Byers wrote:
> Ditto on code duplication from DeviceOrientationDispatcher.cpp

Done.

[Rick Byers, 2015-10-22 16:00:52]

Looks good, just a couple more little suggestions to simplify things a bit
further (and still needs tests before it can land).

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:47:
// FIXME: add user counters as in DeviceOrientationController.
Do you actually want separate use counters here?  Perhaps it's not really worth
tracking the difference between absolute and non-absolute for the purposes of
the UseCounter and RAPPOR metrics. If so you can avoid overriding this function
at all too.

If we decide we do want some measure of what fraction of device orientation
usage is absolute vs. non-absolute, that would still be easy to add in
DeviceOrientationController::didAddEventListener without copying the rest of the
logic (eg. just key it off of the eventType virtual).

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:40:
blink::WebPlatformEventType getWebPlatformEventType(bool absolute)
nit: make this a private const method of the class then you can just read
m_absolute rather than having to pass it in explicitly.

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:52:
DEFINE_STATIC_LOCAL(Persistent<DeviceOrientationDispatcher>,
deviceOrientationAbsoluteDispatcher, (new DeviceOrientationDispatcher(true)));
rather than use template specialization here, wouldn't it be simpler to just
take the bool as an argument to instance and have two DEFINE_STATIC_LOCALs in
that one function?  Then just return one or the other depending on the argument.

[timvolodine, 2015-10-27 16:38:35]

replies to comments, plus also added some layout testing (with a bug fix in
content/browser), ptal

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:47:
// FIXME: add user counters as in DeviceOrientationController.
On 2015/10/22 16:00:52, Rick Byers wrote:
> Do you actually want separate use counters here?  Perhaps it's not really
worth
> tracking the difference between absolute and non-absolute for the purposes of
> the UseCounter and RAPPOR metrics. If so you can avoid overriding this
function
> at all too.
> 
> If we decide we do want some measure of what fraction of device orientation
> usage is absolute vs. non-absolute, that would still be easy to add in
> DeviceOrientationController::didAddEventListener without copying the rest of
the
> logic (eg. just key it off of the eventType virtual).

Yes, well, I think it may actually be interesting to count the usage of the
absolute/non-absolute orientation events separately. The idea is that
non-absolute orientation would be useful for VR and the absolute orientation for
AR applications.

I could refactor the common bits into the base class, but the logging would be
different anyway and I didn't want to pollute the existing counters..

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:40:
blink::WebPlatformEventType getWebPlatformEventType(bool absolute)
On 2015/10/22 16:00:52, Rick Byers wrote:
> nit: make this a private const method of the class then you can just read
> m_absolute rather than having to pass it in explicitly.

Done.

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:52:
DEFINE_STATIC_LOCAL(Persistent<DeviceOrientationDispatcher>,
deviceOrientationAbsoluteDispatcher, (new DeviceOrientationDispatcher(true)));
On 2015/10/22 16:00:52, Rick Byers wrote:
> rather than use template specialization here, wouldn't it be simpler to just
> take the bool as an argument to instance and have two DEFINE_STATIC_LOCALs in
> that one function?  Then just return one or the other depending on the
argument.

yes, looks like using a template is a bit of overkill here ;)
Done.

[Rick Byers, 2015-10-27 18:35:07]

Tests look fine, thanks.

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:47:
// FIXME: add user counters as in DeviceOrientationController.
On 2015/10/27 16:38:35, timvolodine wrote:
> On 2015/10/22 16:00:52, Rick Byers wrote:
> > Do you actually want separate use counters here?  Perhaps it's not really
> worth
> > tracking the difference between absolute and non-absolute for the purposes
of
> > the UseCounter and RAPPOR metrics. If so you can avoid overriding this
> function
> > at all too.
> > 
> > If we decide we do want some measure of what fraction of device orientation
> > usage is absolute vs. non-absolute, that would still be easy to add in
> > DeviceOrientationController::didAddEventListener without copying the rest of
> the
> > logic (eg. just key it off of the eventType virtual).
> 
> Yes, well, I think it may actually be interesting to count the usage of the
> absolute/non-absolute orientation events separately. The idea is that
> non-absolute orientation would be useful for VR and the absolute orientation
for
> AR applications.
> 
> I could refactor the common bits into the base class, but the logging would be
> different anyway and I didn't want to pollute the existing counters..

Ok.  Adding the UseCounter should be trivial.  If it's interesting to have, why
not just do it now?  The rappor logging may be more involved (I'm not familiar
with the code changes required for that), I'm fine with a FIXME there.

[timvolodine, 2015-10-28 17:48:08]

added UseCounter bits.

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:47:
// FIXME: add user counters as in DeviceOrientationController.
On 2015/10/27 18:35:07, Rick Byers wrote:
> On 2015/10/27 16:38:35, timvolodine wrote:
> > On 2015/10/22 16:00:52, Rick Byers wrote:
> > > Do you actually want separate use counters here?  Perhaps it's not really
> > worth
> > > tracking the difference between absolute and non-absolute for the purposes
> of
> > > the UseCounter and RAPPOR metrics. If so you can avoid overriding this
> > function
> > > at all too.
> > > 
> > > If we decide we do want some measure of what fraction of device
orientation
> > > usage is absolute vs. non-absolute, that would still be easy to add in
> > > DeviceOrientationController::didAddEventListener without copying the rest
of
> > the
> > > logic (eg. just key it off of the eventType virtual).
> > 
> > Yes, well, I think it may actually be interesting to count the usage of the
> > absolute/non-absolute orientation events separately. The idea is that
> > non-absolute orientation would be useful for VR and the absolute orientation
> for
> > AR applications.
> > 
> > I could refactor the common bits into the base class, but the logging would
be
> > different anyway and I didn't want to pollute the existing counters..
> 
> Ok.  Adding the UseCounter should be trivial.  If it's interesting to have,
why
> not just do it now?  The rappor logging may be more involved (I'm not familiar
> with the code changes required for that), I'm fine with a FIXME there.

Done. (rappor logging is a todo).

[timvolodine, 2015-10-28 17:51:15]

timvolodine@chromium.org changed reviewers:
+ isherman@chromium.org

[timvolodine, 2015-10-28 17:51:15]

+isherman@: for histograms.xml

(I ran the update_use_counter_feature_enum.py which generated an updated version
of histograms.xml. Looks like somebody forgot to run it when adding
"V8Permissions_RequestAll_Method" which is probably why it's in this change)

[timvolodine, 2015-10-28 17:55:49]

timvolodine@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[timvolodine, 2015-10-28 17:55:49]

+oilpan-reviews@:
could somebody familiar with oilpan please take a look at
DeviceOrientationController and the deriving
DeviceOrientationAbsoluteController? I am not entirely sure it's correct wrt
blink memory management.

[Ilya Sherman, 2015-10-28 18:15:50]

histograms.xml lgtm

[haraken, 2015-10-28 18:18:37]

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

[haraken, 2015-10-28 18:18:38]

Would you compile the CL with GYP_DEFINES='enable_oilpan=1 blink_gc_plugin=1'?
It will catch Oilpan's programming errors.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:49:
// FIXME: add rappor logging of insecure origins as in
DeviceOrientationController.

FIXME => TODO

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:55:
// FIXME: add rappor url logging as in DeviceOrientationController.

FIXME => TODO

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:73:
WillBeHeapSupplement<Document>::trace(visitor);

This is not needed. DeviceOrientationAbsoluteController does not (directly)
inherit from WillBeHeapSupplement<Document>.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.h
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.h:15:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(DeviceOrientationAbsoluteController);

This is not needed because DeviceOrientationAbsoluteController doesn't inherit
from GarbageCollectedMixin.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:43:
DEFINE_STATIC_LOCAL(Persistent<DeviceOrientationDispatcher>,
deviceOrientationAbsoluteDispatcher, (new
DeviceOrientationDispatcher(absolute)));

Shouldn't this be DeviceOrientationAbsoluteDispatcher?

[haraken, 2015-10-28 18:23:34]

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:28:
DeviceOrientationAbsoluteController&
DeviceOrientationAbsoluteController::from(Document& document)

I think this method should just redirect to DeviceOrientationController::from.
Then you can remove DeviceOrientationAbsoluteController::supplementName().

Basically it looks strange that you have to define supplementName() in multiple
classes in one hierarchy.

class A : public HeapSupplement<Document> {
  const char* supplementName();
};

class B : public A {
  const char* supplementName();  // This doesn't make much sense. Since A and B
are the same object, it is enough to define A::supplementName() and call
Document::provideTo() only in A::from().
};

[Rick Byers, 2015-10-28 18:27:51]

LGTM, thanks (subject to the oilpan review of course)

[timvolodine, 2015-10-29 13:49:19]

haraken@: thanks for the comments! replies below.

I've tried compiling with
build/gyp_chromium -G config=Debug -D component=shared_library -D OS=android -D
enable_oilpan=1 -D blink_gc_plugin=1

but get an error even on master branch:

[..]
In file included from
../../third_party/WebKit/Source/platform/LifecycleNotifier.h:30:0,
                 from
../../third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.h:32,
                 from
../../third_party/WebKit/Source/core/dom/ExecutionContext.h:32,
                 from
../../third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:31,
                 from
../../third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.h:35,
                 from
gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.h:10,
                 from
gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.cpp:8,
                 from
gen/blink/bindings/core/v8/V8GeneratedCoreBindings14.cpp:31:
../../third_party/WebKit/Source/platform/heap/Handle.h:1038:37: error: type
attributes ignored after type is already defined [-Werror=attributes]
 #define WillBeHeapSupplement blink::HeapSupplement
                                     ^
../../third_party/WebKit/Source/core/dom/Document.h:1397:51: note: in expansion
of macro 'WillBeHeapSupplement'
 extern template class CORE_EXTERN_TEMPLATE_EXPORT
WillBeHeapSupplement<Document>;
                                                   ^
cc1plus: all warnings being treated as errors
[172/2708] CXX
obj/third_party/WebKit/Source/core/css/webcore_shared.StyleSheetContents.o

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:28:
DeviceOrientationAbsoluteController&
DeviceOrientationAbsoluteController::from(Document& document)
On 2015/10/28 18:23:34, haraken wrote:
> 
> I think this method should just redirect to DeviceOrientationController::from.
> Then you can remove DeviceOrientationAbsoluteController::supplementName().
> 
> Basically it looks strange that you have to define supplementName() in
multiple
> classes in one hierarchy.
> 
> class A : public HeapSupplement<Document> {
>   const char* supplementName();
> };
> 
> class B : public A {
>   const char* supplementName();  // This doesn't make much sense. Since A and
B
> are the same object, it is enough to define A::supplementName() and call
> Document::provideTo() only in A::from().
> };

Hmm, not sure how this would work. The problem is that
DeviceOrientationController and DeviceOrientationAbsoluteController can both be
supplements of the same document at the same time, so I guess we really need a
different supplement name for this to work..

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:49:
// FIXME: add rappor logging of insecure origins as in
DeviceOrientationController.
On 2015/10/28 18:18:37, haraken wrote:
> 
> FIXME => TODO

Done.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:55:
// FIXME: add rappor url logging as in DeviceOrientationController.
On 2015/10/28 18:18:37, haraken wrote:
> 
> FIXME => TODO

Done.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:73:
WillBeHeapSupplement<Document>::trace(visitor);
On 2015/10/28 18:18:37, haraken wrote:
> 
> This is not needed. DeviceOrientationAbsoluteController does not (directly)
> inherit from WillBeHeapSupplement<Document>.

Done.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.h
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.h:15:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(DeviceOrientationAbsoluteController);
On 2015/10/28 18:18:38, haraken wrote:
> 
> This is not needed because DeviceOrientationAbsoluteController doesn't inherit
> from GarbageCollectedMixin.

Done.

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:43:
DEFINE_STATIC_LOCAL(Persistent<DeviceOrientationDispatcher>,
deviceOrientationAbsoluteDispatcher, (new
DeviceOrientationDispatcher(absolute)));
On 2015/10/28 18:18:38, haraken wrote:
> 
> Shouldn't this be DeviceOrientationAbsoluteDispatcher?

no, we've only one dispatcher class (parameterized by 'absolute') to save some
code.

[haraken, 2015-10-29 13:56:48]

On 2015/10/29 13:49:19, timvolodine wrote:
> haraken@: thanks for the comments! replies below.
> 
> I've tried compiling with
> build/gyp_chromium -G config=Debug -D component=shared_library -D OS=android
-D
> enable_oilpan=1 -D blink_gc_plugin=1
> 
> but get an error even on master branch:
> 
> [..]
> In file included from
> ../../third_party/WebKit/Source/platform/LifecycleNotifier.h:30:0,
>                  from
> ../../third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.h:32,
>                  from
> ../../third_party/WebKit/Source/core/dom/ExecutionContext.h:32,
>                  from
> ../../third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:31,
>                  from
> ../../third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.h:35,
>                  from
> gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.h:10,
>                  from
> gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.cpp:8,
>                  from
> gen/blink/bindings/core/v8/V8GeneratedCoreBindings14.cpp:31:
> ../../third_party/WebKit/Source/platform/heap/Handle.h:1038:37: error: type
> attributes ignored after type is already defined [-Werror=attributes]
>  #define WillBeHeapSupplement blink::HeapSupplement
>                                      ^
> ../../third_party/WebKit/Source/core/dom/Document.h:1397:51: note: in
expansion
> of macro 'WillBeHeapSupplement'
>  extern template class CORE_EXTERN_TEMPLATE_EXPORT
> WillBeHeapSupplement<Document>;
>                                                    ^
> cc1plus: all warnings being treated as errors
> [172/2708] CXX
> obj/third_party/WebKit/Source/core/css/webcore_shared.StyleSheetContents.o

Is the error happening on ToT? I can compile 356833 with enable_oilpan=1.

[timvolodine, 2015-10-29 13:59:33]

On 2015/10/29 13:56:48, haraken wrote:
> On 2015/10/29 13:49:19, timvolodine wrote:
> > haraken@: thanks for the comments! replies below.
> > 
> > I've tried compiling with
> > build/gyp_chromium -G config=Debug -D component=shared_library -D OS=android
> -D
> > enable_oilpan=1 -D blink_gc_plugin=1
> > 
> > but get an error even on master branch:
> > 
> > [..]
> > In file included from
> > ../../third_party/WebKit/Source/platform/LifecycleNotifier.h:30:0,
> >                  from
> > ../../third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.h:32,
> >                  from
> > ../../third_party/WebKit/Source/core/dom/ExecutionContext.h:32,
> >                  from
> > ../../third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:31,
> >                  from
> > ../../third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.h:35,
> >                  from
> > gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.h:10,
> >                  from
> > gen/blink/bindings/core/v8/V8FontFaceSetForEachCallback.cpp:8,
> >                  from
> > gen/blink/bindings/core/v8/V8GeneratedCoreBindings14.cpp:31:
> > ../../third_party/WebKit/Source/platform/heap/Handle.h:1038:37: error: type
> > attributes ignored after type is already defined [-Werror=attributes]
> >  #define WillBeHeapSupplement blink::HeapSupplement
> >                                      ^
> > ../../third_party/WebKit/Source/core/dom/Document.h:1397:51: note: in
> expansion
> > of macro 'WillBeHeapSupplement'
> >  extern template class CORE_EXTERN_TEMPLATE_EXPORT
> > WillBeHeapSupplement<Document>;
> >                                                    ^
> > cc1plus: all warnings being treated as errors
> > [172/2708] CXX
> > obj/third_party/WebKit/Source/core/css/webcore_shared.StyleSheetContents.o
> 
> Is the error happening on ToT? I can compile 356833 with enable_oilpan=1.

this is my regular clankium checkout actually, not ToT.

[haraken, 2015-10-29 14:02:28]

https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:28:
> DeviceOrientationAbsoluteController&
> DeviceOrientationAbsoluteController::from(Document& document)
> On 2015/10/28 18:23:34, haraken wrote:
> > 
> > I think this method should just redirect to
DeviceOrientationController::from.
> > Then you can remove DeviceOrientationAbsoluteController::supplementName().
> > 
> > Basically it looks strange that you have to define supplementName() in
> multiple
> > classes in one hierarchy.
> > 
> > class A : public HeapSupplement<Document> {
> >   const char* supplementName();
> > };
> > 
> > class B : public A {
> >   const char* supplementName();  // This doesn't make much sense. Since A
and
> B
> > are the same object, it is enough to define A::supplementName() and call
> > Document::provideTo() only in A::from().
> > };
> 
> Hmm, not sure how this would work. The problem is that
> DeviceOrientationController and DeviceOrientationAbsoluteController can both
be
> supplements of the same document at the same time, so I guess we really need a
> different supplement name for this to work..

Do you mean that Document::m_supplements (more accurately,
SupplementableTracing::m_supplements) can contain both:

  "DeviceOrientationController" => OwnPtr<DeviceOrientationController> 
  "DeviceOrientationAbsoluteController" => 
OwnPtr<DeviceOrientationAbsoluteController>

at the same time (i.e., the two OwnPtrs are pointing to the same object)?

I don't think it works.

[timvolodine, 2015-10-29 14:09:42]

On 2015/10/29 14:02:28, haraken wrote:
>
https://codereview.chromium.org/1416123002/diff/180001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/modules/device_orientation/DeviceOrientationAbsoluteController.cpp:28:
> > DeviceOrientationAbsoluteController&
> > DeviceOrientationAbsoluteController::from(Document& document)
> > On 2015/10/28 18:23:34, haraken wrote:
> > > 
> > > I think this method should just redirect to
> DeviceOrientationController::from.
> > > Then you can remove DeviceOrientationAbsoluteController::supplementName().
> > > 
> > > Basically it looks strange that you have to define supplementName() in
> > multiple
> > > classes in one hierarchy.
> > > 
> > > class A : public HeapSupplement<Document> {
> > >   const char* supplementName();
> > > };
> > > 
> > > class B : public A {
> > >   const char* supplementName();  // This doesn't make much sense. Since A
> and
> > B
> > > are the same object, it is enough to define A::supplementName() and call
> > > Document::provideTo() only in A::from().
> > > };
> > 
> > Hmm, not sure how this would work. The problem is that
> > DeviceOrientationController and DeviceOrientationAbsoluteController can both
> be
> > supplements of the same document at the same time, so I guess we really need
a
> > different supplement name for this to work..
> 
> Do you mean that Document::m_supplements (more accurately,
> SupplementableTracing::m_supplements) can contain both:
> 
>   "DeviceOrientationController" => OwnPtr<DeviceOrientationController> 
>   "DeviceOrientationAbsoluteController" => 
> OwnPtr<DeviceOrientationAbsoluteController>
> 
> at the same time (i.e., the two OwnPtrs are pointing to the same object)?
> 
> I don't think it works.

Not the same object though "DeviceOrientationAbsoluteController" would point to
an instance of DeviceOrientationAbsoluteController. I thought the supplements
are keyed by their name no? The ::from methods for both classes are explicitly
called in FrameLoaderClientImpl.cpp.

[haraken, 2015-10-29 14:15:11]

https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:144:
DeviceOrientationAbsoluteController::from(*document);

If the runtime flag is enabled, won't it happen that the same object is
double-registered to the Document's supplement map?

It will cause a problem because when the Document dies, the Document calls
destructors for all supplement objects in the map. If the same object is
double-registered, the second destructor call will cause double-free.

That's why I'm concerned about implementing supplementName() on multiple classes
in the same hierarchy.

[timvolodine, 2015-10-29 19:34:33]

https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:144:
DeviceOrientationAbsoluteController::from(*document);
On 2015/10/29 14:15:11, haraken wrote:
> 
> If the runtime flag is enabled, won't it happen that the same object is
> double-registered to the Document's supplement map?
> 
> It will cause a problem because when the Document dies, the Document calls
> destructors for all supplement objects in the map. If the same object is
> double-registered, the second destructor call will cause double-free.
> 
> That's why I'm concerned about implementing supplementName() on multiple
classes
> in the same hierarchy.

If understand correctly the point about supplementName() is that we are actually
hiding the super class methods. If somebody static_casts the 'absolute'
controller to its super class and then invokes supplementName() it would give
the name of the super class.

However I don't see this kind of usage in current code and don't think it's
possible to have the same object twice in the map.

If having a supplementName as part of class is a concern perhaps a solution
could be to put it into anonymous namespace in the .cpp... or extract a common
DeviceOrientationBase class and make the others derive from it while
implementing static methods. wdyt?

[haraken, 2015-10-29 20:00:33]

On 2015/10/29 19:34:33, timvolodine wrote:
>
https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:144:
> DeviceOrientationAbsoluteController::from(*document);
> On 2015/10/29 14:15:11, haraken wrote:
> > 
> > If the runtime flag is enabled, won't it happen that the same object is
> > double-registered to the Document's supplement map?
> > 
> > It will cause a problem because when the Document dies, the Document calls
> > destructors for all supplement objects in the map. If the same object is
> > double-registered, the second destructor call will cause double-free.
> > 
> > That's why I'm concerned about implementing supplementName() on multiple
> classes
> > in the same hierarchy.
> 
> If understand correctly the point about supplementName() is that we are
actually
> hiding the super class methods. If somebody static_casts the 'absolute'
> controller to its super class and then invokes supplementName() it would give
> the name of the super class.
> 
> However I don't see this kind of usage in current code and don't think it's
> possible to have the same object twice in the map.

Yeah, thanks for the clarification. Now I understand I'm just concerned about
hypothetical cases that won't happen in reality. Sorry about the noise :/

LGTM.

[timvolodine, 2015-11-02 15:34:09]

On 2015/10/29 20:00:33, haraken wrote:
> On 2015/10/29 19:34:33, timvolodine wrote:
> >
>
https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1416123002/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:144:
> > DeviceOrientationAbsoluteController::from(*document);
> > On 2015/10/29 14:15:11, haraken wrote:
> > > 
> > > If the runtime flag is enabled, won't it happen that the same object is
> > > double-registered to the Document's supplement map?
> > > 
> > > It will cause a problem because when the Document dies, the Document calls
> > > destructors for all supplement objects in the map. If the same object is
> > > double-registered, the second destructor call will cause double-free.
> > > 
> > > That's why I'm concerned about implementing supplementName() on multiple
> > classes
> > > in the same hierarchy.
> > 
> > If understand correctly the point about supplementName() is that we are
> actually
> > hiding the super class methods. If somebody static_casts the 'absolute'
> > controller to its super class and then invokes supplementName() it would
give
> > the name of the super class.
> > 
> > However I don't see this kind of usage in current code and don't think it's
> > possible to have the same object twice in the map.
> 
> Yeah, thanks for the clarification. Now I understand I'm just concerned about
> hypothetical cases that won't happen in reality. Sorry about the noise :/
> 
> LGTM.

ok,
thanks for the reviews everybody!

[timvolodine, 2015-11-02 15:34:16]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-11-02 15:34:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rbyers@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1416123002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-02 15:34:49]

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

[timvolodine, 2015-11-02 15:35:13]

The CQ bit was unchecked by timvolodine@chromium.org

[timvolodine, 2015-11-02 15:35:46]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.

TODO: Add layout tests once crrev.com/1402923004 lands.

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event. Also adds new
layout tests.

BUG=520546
==========

[timvolodine, 2015-11-02 15:36:35]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event. Also adds new
layout tests.

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add new layout tests.

BUG=520546
==========

[timvolodine, 2015-11-02 15:37:43]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add new layout tests.

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add new layout tests and UseCounters.

BUG=520546
==========

[timvolodine, 2015-11-02 15:42:10]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add new layout tests and UseCounters.

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add and update layout tests and UseCounters.
The 'deviceorientationabsolute' event listener is currently exposed
behind the experimental features flag.

BUG=520546
==========

[timvolodine, 2015-11-02 15:43:04]

Description was changed from

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add and update layout tests and UseCounters.
The 'deviceorientationabsolute' event listener is currently exposed
behind the experimental features flag.

BUG=520546
==========

to

==========
Absolute Device Orientation API: blink implementation.

Implement blink classes for the 'deviceorientationabsolute' event.
Also add and update layout tests and UseCounters.
The 'ondeviceorientationabsolute' event listener is currently
exposed behind the experimental features flag.

BUG=520546
==========

[timvolodine, 2015-11-02 15:43:44]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-11-02 15:44:14]

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

[commit-bot: I haz the power, 2015-11-02 16:51:59]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-02 16:53:00]

Patchset 13 (id:??) landed as
https://crrev.com/70e4cfe9b1a1ed76535b5b846dccef9033418313
Cr-Commit-Position: refs/heads/master@{#357374}