Bug 43258 – Implement chromium WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Hans Wennborg]

WebViewImpl must provide Page with a DeviceOrientationClient. It should do this by asking WebViewClient for a WebDeviceOrientationClient.

[Hans Wennborg]

Created attachment 63069 [details]
Patch

[Jeremy Orlow]

Comment on attachment 63069 [details]
Patch

WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
 +      // WEBKIT_API setOrientation(const WebDeviceOrientation&);
These should be virtual thus WEBKIT_API is not needed.  All of these will also be in the WebDeviceOrientationClient.h (and be virtual).  The methods in the cpp should just call m_private->method_name(...)

WebKit/chromium/public/WebDeviceOrientationClientMock.h:28
 +  #include "WebCommon.h"
not needed

WebKit/chromium/public/WebDeviceOrientationClientMock.h:48
 +      WebCore::DeviceOrientationClient* m_private;
Use the private ptr class instead.

WebKit/chromium/public/WebDeviceOrientationClientMock.h:37
 +      WEBKIT_API WebDeviceOrientationClientMock();
Define these inline.  Have them both call initialize() and have it set itself to a new mock object in the src file.  Have the destructor call reset() and define it as {} in the src file.

WebDeviceOrientationCLient.cpp doesn't need to exist.  All the methods in the .h file should be implemented as calling WEBKIT_ASSERT_NOT_REACHED().

WebKit/chromium/src/WebViewImpl.cpp:266
 +      , m_webDeviceOrientationClient(createWebDeviceOrientationClient(client))
m_webDeviceOrientationClient(webViewClient->createWebDeviceOrientationClient())

WebKit/chromium/src/WebViewImpl.cpp:291
 +          pageClients.deviceOrientationClient = m_webDeviceOrientationClient->client();
not needed

WebKit/chromium/src/WebViewImpl.cpp:2211
 +  WebDeviceOrientationClient* WebViewImpl::createWebDeviceOrientationClient(WebViewClient* webViewClient)
not needed




You're almost there.

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 63069 [details])
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
>  +      // WEBKIT_API setOrientation(const WebDeviceOrientation&);
> These should be virtual thus WEBKIT_API is not needed.  All of these will also be in the WebDeviceOrientationClient.h (and be virtual).  The methods in the cpp should just call m_private->method_name(...)
But it's only the mock client that has a setOrientation function, so I don't see why it should be virtual?

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:28
>  +  #include "WebCommon.h"
> not needed
OK.

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:48
>  +      WebCore::DeviceOrientationClient* m_private;
> Use the private ptr class instead.
DeviceOrientationClient is not reference counted. As I understand, WebPrivatePtr is for reference counted types?

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:37
>  +      WEBKIT_API WebDeviceOrientationClientMock();
> Define these inline.  Have them both call initialize() and have it set itself to a new mock object in the src file.
Will do.

> Have the destructor call reset() and define it as {} in the src file.
Wait; both call reset() and define as {}?

> 
> WebDeviceOrientationCLient.cpp doesn't need to exist.  All the methods in the .h file should be implemented as calling WEBKIT_ASSERT_NOT_REACHED().
But isn't it a good defensive strategy to provide a dummy implementation for when WebViewClient fails to provide a WebDeviceOrientationClient?

> 
> WebKit/chromium/src/WebViewImpl.cpp:266
>  +      , m_webDeviceOrientationClient(createWebDeviceOrientationClient(client))
> m_webDeviceOrientationClient(webViewClient->createWebDeviceOrientationClient())
What if webViewClient is NULL? As Satish pointed out, it sometimes is.

> 
> WebKit/chromium/src/WebViewImpl.cpp:291
>  +          pageClients.deviceOrientationClient = m_webDeviceOrientationClient->client();
> not needed
> 
> WebKit/chromium/src/WebViewImpl.cpp:2211
>  +  WebDeviceOrientationClient* WebViewImpl::createWebDeviceOrientationClient(WebViewClient* webViewClient)
> not needed
> 
> 
> 
> 
> You're almost there.

[Hans Wennborg]

> > Have the destructor call reset() and define it as {} in the src file.
> Wait; both call reset() and define as {}?

Sorry, I should have read that one more time. You obviously mean have the destructor call reset, and define *reset* as {}.

[Hans Wennborg]

Created attachment 63207 [details]
Patch

Moving the dummy implementation to its own class.

Hiding constructors and providing create() methods. Otherwise, the compilation unit constructing the object may not have WEBKIT_IMPLEMENTATION set, won't see the virtual method and thus not set up the vtable for it.

[Hans Wennborg]

Created attachment 63312 [details]
Patch

[Jeremy Orlow]

Comment on attachment 63312 [details]
Patch

WebKit/chromium/src/WebDeviceOrientationClientDummy.cpp:37
 +      DeviceOrientationClientDummy() : orientation(WebCore::DeviceOrientation::create()) { }
usually we split onto 2 lines right before the :

WebKit/chromium/public/WebDeviceOrientation.h:32
 +  #include <wtf/PassRefPtr.h>
You can just do "namespace WTF { template <typename T> class PassRefPtr; }"

WebKit/chromium/public/WebDeviceOrientation.h:43
 +          initialize(canProvideAlpha, alpha, canProvideBeta, beta, canProvideGamma, gamma);
Darin, in the past you've been pretty adamant that constructors be inline.  Do you think that doing it this way is better than the alternative?  I'm not so sure.

WebKit/chromium/public/WebDeviceOrientation.h:48
 +      operator PassRefPtr<WebCore::DeviceOrientation>() const;
add the other 2 operators we have in just about every other class like this

WebKit/chromium/public/WebDeviceOrientationClient.h:50
 +      WebDeviceOrientationClient() { }
This works??  I'm pretty sure this will need to be public and virtual.

WebKit/chromium/src/WebViewImpl.cpp:267
 +      , m_webDeviceOrientationClient(WebRuntimeFeatures::isDeviceOrientationEnabled() ? (client ? client->createWebDeviceOrientationClient() : WebDeviceOrientationClientDummy::create()) : 0)
What is the dummy for?  I don't think client can ever be non-null.  And, if it can be, then we should just assume the device orientation client is null too then.

I don't see any point to the dummy.  I'd rather not add logic to WebKit to handle the client and DOclient being null unless there's already precedent.  But even if so, I think that's better than having some dummy client.

Another option, if all else fails, you could just allocate the mock.


hmmm...lets just talk in person

[Jeremy Orlow]

Talked to hans some about this.  I think we're on the same page now.  I think right now he's leaning towards just putting the two DO callbacks directly into the WebViewClient and then having the DRT version of it call back into WebKit mock which will simply proxy calls to the WebCore mock.  (And then put a fixme in that if it grows beyond just 2 methods, it should be pulled out into its own client.)

[Hans Wennborg]

Fixing style issues first, will then provide a code example of how this would tie in with the orientation plumbing as a whole to make this easier to discuss.

(In reply to comment #7)
> (From update of attachment 63312 [details])
> WebKit/chromium/src/WebDeviceOrientationClientDummy.cpp:37
>  +      DeviceOrientationClientDummy() : orientation(WebCore::DeviceOrientation::create()) { }
> usually we split onto 2 lines right before the :
Done.

> 
> WebKit/chromium/public/WebDeviceOrientation.h:32
>  +  #include <wtf/PassRefPtr.h>
> You can just do "namespace WTF { template <typename T> class PassRefPtr; }"
Done.

> 
> WebKit/chromium/public/WebDeviceOrientation.h:43
>  +          initialize(canProvideAlpha, alpha, canProvideBeta, beta, canProvideGamma, gamma);
> Darin, in the past you've been pretty adamant that constructors be inline.  Do you think that doing it this way is better than the alternative?  I'm not so sure.
Happy to change this if you decide it needn't be inline.

> 
> WebKit/chromium/public/WebDeviceOrientation.h:48
>  +      operator PassRefPtr<WebCore::DeviceOrientation>() const;
> add the other 2 operators we have in just about every other class like this
Done. (I assume you mean assignment operator and copy ctor.)

> 
> WebKit/chromium/public/WebDeviceOrientationClient.h:50
>  +      WebDeviceOrientationClient() { }
> This works??  I'm pretty sure this will need to be public and virtual.
AFAIK, constructors can't be virtual. Why would it need to be public? It's only called by subclass constructors (and create(), when that is implemented).

> 
> WebKit/chromium/src/WebViewImpl.cpp:267
>  +      , m_webDeviceOrientationClient(WebRuntimeFeatures::isDeviceOrientationEnabled() ? (client ? client->createWebDeviceOrientationClient() : WebDeviceOrientationClientDummy::create()) : 0)
> What is the dummy for?  I don't think client can ever be non-null.  And, if it can be, then we should just assume the device orientation client is null too then.
client will be null during some UI tests. This check is analogue to Satish's check in Bug 43240. Either we handle it, or we require it to be non-NULL.

We can't let device orientation be NULL when the feature is enabled, as Page will then crash upon construction.

> I don't see any point to the dummy.  I'd rather not add logic to WebKit to handle the client and DOclient being null unless there's already precedent.  But even if so, I think that's better than having some dummy client.
> 
> Another option, if all else fails, you could just allocate the mock.

Will scrap the dummy and use the mock as fall-back for now. Willing to change later.

[Jeremy Orlow]

(In reply to comment #9)
> > WebKit/chromium/public/WebDeviceOrientationClient.h:50
> >  +      WebDeviceOrientationClient() { }
> > This works??  I'm pretty sure this will need to be public and virtual.
> AFAIK, constructors can't be virtual. Why would it need to be public? It's only called by subclass constructors (and create(), when that is implemented).

Oops, thought that was a destructor.

> 
> > 
> > WebKit/chromium/src/WebViewImpl.cpp:267
> >  +      , m_webDeviceOrientationClient(WebRuntimeFeatures::isDeviceOrientationEnabled() ? (client ? client->createWebDeviceOrientationClient() : WebDeviceOrientationClientDummy::create()) : 0)
> > What is the dummy for?  I don't think client can ever be non-null.  And, if it can be, then we should just assume the device orientation client is null too then.
> client will be null during some UI tests. This check is analogue to Satish's check in Bug 43240. Either we handle it, or we require it to be non-NULL.

Can we make that not be true easily?  Or can those tests not set the flag?

[Hans Wennborg]

Created attachment 63432 [details]
WebDeviceOrientationClient plus plumbing

Attaching patch showing how this would tie in with the plumbing.

The interesting classes are WebView, WebViewImpl, WebViewClient, and DeviceOrientationClientImpl.


WebViewClient.h: createWebDeviceOrientationClient(). Called by the WebViewImpl constructor. When implemented by RenderView, this will return a WebDeviceOrientationClientImpl instance. When implemented by DRT, it will return a WebDeviceOrientationClientMock.

WebViewImpl.h: It owns a WebDeviceOrientationClient. This will either be the mock or the real implementation.

WebView.h: didChangeDeviceOrientation(). This is where orientation data is passed from Chromium to the WebKit side. This is used both for real data and mock data. WebViewImpl which implments this does not care whether it's mock or real data, it just forwards it to its WebDeviceOrientationClient.

WebViewClient.h: {start,stop}DeviceOrientationUpdates(). These are called by the DeviceOrientationClientImpl.

Note that communication with the DeviceOrientationClientMock is one-way: it receives mock orientation updates, but it doesn't call into the embedder.

[Jeremy Orlow]

Comment on attachment 63432 [details]
WebDeviceOrientationClient plus plumbing

WebKit/chromium/public/WebViewClient.h:94
 +      virtual WebDeviceOrientationClient* createWebDeviceOrientationClient() { return WebDeviceOrientationClientMock::create(); }
Remove.  Just call WebDeviceOrientationClientMock::create directly.

WebKit/chromium/src/WebViewImpl.h:526
 +      OwnPtr<WebDeviceOrientationClient> m_webDeviceOrientationClient;
delete

WebKit/chromium/src/WebViewImpl.h:79
 +  class WebDeviceOrientationClient;
delete

WebKit/chromium/src/WebViewImpl.cpp:268
 +      , m_webDeviceOrientationClient(WebRuntimeFeatures::isDeviceOrientationEnabled() ? (client ? client->createWebDeviceOrientationClient() : WebDeviceOrientationClientMock::create()) : 0)
delete

WebKit/chromium/src/WebViewImpl.cpp:292
 +      if (m_webDeviceOrientationClient)
this is all obsolete

WebKit/chromium/src/WebViewImpl.cpp:2216
 +          m_webDeviceOrientationClient->updateOrientation(o);
The WebViewImpl should be calling some web core method directly to tell it of the update.


I believe the only new files you should have in this patch are WebDeviceOrientationClientMock.cpp/h and DeviceOrientationClientProxy (which will have a pointer to the WebViewClient and pass stuff on to that).  If you need anything more I suggest you run it by me first.

I'd highly suggest you get all of this compiling/working together on a local machine before posting another patch.  It'll help clarify the design, I think.

[Jeremy Orlow]

Another option is that you might want to try doing the full scale design we talked about, because it should be much more clear.  You can then scale it down if you'd like.  The design of the scaled down version will be much more obvious if you understand the scaled up version.

[Hans Wennborg]

(In reply to comment #13)
> Another option is that you might want to try doing the full scale design we talked about, because it should be much more clear.  You can then scale it down if you'd like.  The design of the scaled down version will be much more obvious if you understand the scaled up version.

Uploading patch with the full scale design we discussed. There are probably some rough corners as there is a lot of new code, but hopefully the overall design is more clear.

[Hans Wennborg]

Created attachment 63598 [details]
Patch

[Hans Wennborg]

Turns out the comment in WebViewImpl about Page taking ownership of the various clients it gets passed in the PageClients structure is not true.

Uploading new patch where WebViewImpl takes ownership of the DeviceOrientationClientProxy.

[Hans Wennborg]

Created attachment 63717 [details]
Patch

[Jeremy Orlow]

Comment on attachment 63717 [details]
Patch

WebKit/chromium/src/WebViewImpl.cpp:290
 +          m_deviceOrientationClientProxy.set(new DeviceOrientationClientProxy(client ? client->deviceOrientationClient() : 0));
See if you can clean up this logic a bit.  It's kinda weird how we have this matrix of possibilities between whether the flag is on and whether there's a client.  

WebKit/chromium/src/WebDeviceOrientationController.cpp:31
 +  #include "PassRefPtr.h"
<wtf/PassRefPtr.h>

WebKit/chromium/src/WebDeviceOrientationController.cpp:36
 +  void WebDeviceOrientationController::didChangeDeviceOrientation(const WebDeviceOrientation& o)
use a better variable name

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:68
 +      delete m_clientMock;
and set it to 0...and first do an if to see if it's already 0

and maybe you should just make a class much like PrivatePtr...make that PrivateRefPtr and then add PrivateOwnPtr?  Just a thought.

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:63
 +      m_clientMock = new WebCore::DeviceOrientationClientMock();
Hmmm...i'm tempted to have this call reset() but I guess the constructor doesn't set it to 0 before calling it...so that's not possible.  I guess leave it as is.

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
 +      return lastOrientation ? new WebDeviceOrientation(lastOrientation) : 0;
Hmmm....this should probably just pass back the object, not a pointer to it.  But the problem is that there might not be a last orientation.  Hmmmm....maybe you shoudl add an invalid/null state to the WebDO class?  This is what we've done for serializedScriptValue and a couple others where a null refPtr has semantic meaning.  Not sure about this, but I think it's better than passing around pointers when we don't absolutely have to.

WebKit/chromium/src/WebDeviceOrientation.cpp:49
 +      m_canProvideGamma = o->canProvideGamma();
what about the actual gamma value?

WebKit/chromium/src/WebDeviceOrientation.cpp:39
 +            m_canProvideGamma(o->canProvideGamma())
ditto

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:32
 +  #include "RefPtr.h"
ditto ditto ditto

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:44
 +      if (!m_client)
add fixme about getting rid of these null checks once it's enabled by default

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:68
 +      OwnPtr<WebDeviceOrientation> lastWebOrientation(m_client->lastOrientation());
Hmmm...this probably needs to be a refPtr in WebCore.

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:72
 +      m_lastOrientation = *lastWebOrientation;
Why are we caching it here?  I guess because it was an own ptr?  Well, we should def make it ref.

WebKit/chromium/public/WebDeviceOrientation.h:39
 +              : m_canProvideAlpha(canProvideAlpha),
too many spaces in

WebKit/chromium/public/WebDeviceOrientation.h:46
 +      }
You probably should add methods to this for getting the values.

[Hans Wennborg]

(In reply to comment #18)
> (From update of attachment 63717 [details])
> WebKit/chromium/src/WebViewImpl.cpp:290
>  +          m_deviceOrientationClientProxy.set(new DeviceOrientationClientProxy(client ? client->deviceOrientationClient() : 0));
> See if you can clean up this logic a bit.  It's kinda weird how we have this matrix of possibilities between whether the flag is on and whether there's a client.  
Done. Always initializing m_deviceOrientationClientProxy, but only passing it on to Page is the runtime flag is set.

> 
> WebKit/chromium/src/WebDeviceOrientationController.cpp:31
>  +  #include "PassRefPtr.h"
> <wtf/PassRefPtr.h>
Done.

> 
> WebKit/chromium/src/WebDeviceOrientationController.cpp:36
>  +  void WebDeviceOrientationController::didChangeDeviceOrientation(const WebDeviceOrientation& o)
> use a better variable name
Done.

> 
> WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:68
>  +      delete m_clientMock;
> and set it to 0...and first do an if to see if it's already 0
Setting it to 0 after delete. Not sure why I'd need an if before delete? delete 0 just does nothing.

> 
> and maybe you should just make a class much like PrivatePtr...make that PrivateRefPtr and then add PrivateOwnPtr?  Just a thought.
Filed Bug 43709 to do that, but I don't think this patch needs to block on it?

> 
> WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:63
>  +      m_clientMock = new WebCore::DeviceOrientationClientMock();
> Hmmm...i'm tempted to have this call reset() but I guess the constructor doesn't set it to 0 before calling it...so that's not possible.  I guess leave it as is.
OK.

> 
> WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
>  +      return lastOrientation ? new WebDeviceOrientation(lastOrientation) : 0;
> Hmmm....this should probably just pass back the object, not a pointer to it.  But the problem is that there might not be a last orientation.  Hmmmm....maybe you shoudl add an invalid/null state to the WebDO class?  This is what we've done for serializedScriptValue and a couple others where a null refPtr has semantic meaning.  Not sure about this, but I think it's better than passing around pointers when we don't absolutely have to.
Adding null state to WebDeviceOrientation and having WebDOClient::lastOrientation() return an object rather than a pointer.

> 
> WebKit/chromium/src/WebDeviceOrientation.cpp:49
>  +      m_canProvideGamma = o->canProvideGamma();
> what about the actual gamma value?
Oops. Fixed.

> 
> WebKit/chromium/src/WebDeviceOrientation.cpp:39
>  +            m_canProvideGamma(o->canProvideGamma())
> ditto
Fixed.

> 
> WebKit/chromium/src/DeviceOrientationClientProxy.cpp:32
>  +  #include "RefPtr.h"
> ditto ditto ditto
Done.

> 
> WebKit/chromium/src/DeviceOrientationClientProxy.cpp:44
>  +      if (!m_client)
> add fixme about getting rid of these null checks once it's enabled by default
Done.

> 
> WebKit/chromium/src/DeviceOrientationClientProxy.cpp:68
>  +      OwnPtr<WebDeviceOrientation> lastWebOrientation(m_client->lastOrientation());
> Hmmm...this probably needs to be a refPtr in WebCore.
This is obsolete now that WebDOClient::lastOrientation() returns an object instead of a pointer.

> 
> WebKit/chromium/src/DeviceOrientationClientProxy.cpp:72
>  +      m_lastOrientation = *lastWebOrientation;
> Why are we caching it here?  I guess because it was an own ptr?  Well, we should def make it ref.
That's an unpleasant detail. DeviceOrientation::lastOrientation() is expected to return a raw pointer to a DeviceOrientation object, which is reference counted.
The problem is we create the DeviceOrientation object from the WebDeviceOrientation object, and if we don't hold a reference to it ourselves, it will be destructed before our caller gets the result and puts it in a RefPtr. That's why we cache it: to hold an extra reference to it.

If this is too ugly, we should talk to Steve about changing DeviceOrientation::lastOrientation() to return a PassRefPtr.

> 
> WebKit/chromium/public/WebDeviceOrientation.h:39
>  +              : m_canProvideAlpha(canProvideAlpha),
> too many spaces in
Fixed.

> 
> WebKit/chromium/public/WebDeviceOrientation.h:46
>  +      }
> You probably should add methods to this for getting the values.
Done.

[Hans Wennborg]

Created attachment 63884 [details]
Patch

[Jeremy Orlow]

Comment on attachment 63884 [details]
Patch

WebKit/chromium/public/WebDeviceOrientation.h:49
 +      static WebDeviceOrientation nullOrientation() { return WebDeviceOrientation(); }
Since you're using (), C++ guarantees us that everything is 0'ed out?

WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
 +      void setController(WebDeviceOrientationController*);
Either virtual of WEBKIT_API

WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
 +      void initialize();
WEBKIT_API

WebKit/chromium/public/WebDeviceOrientationClientMock.h:52
 +      WebCore::DeviceOrientationClientMock* m_clientMock;
I can't remember if this is allowed or not.  We might prefer a void* and casting...see if you can find other examples (like in PrivatePtr).

WebKit/chromium/src/WebDeviceOrientation.cpp:70
 +          return PassRefPtr<WebCore::DeviceOrientation>(0);
Can't you just say "return 0;" ?

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
 +      return WebDeviceOrientation(lastOrientation);
Why store it to the temp variable...doesn't seem to give you much....and why do you need to cast it explicitly either?  It should just work.

WebKit/chromium/src/WebViewImpl.cpp:290
 +      if (WebRuntimeFeatures::isDeviceOrientationEnabled())
Can't you do this either way?  Worst case, it gets set to null which is what would have happened anyway.


Minor comments so r=me, but you're not a committer so you'll need to post another rev.  Let me know if you only address stuff I mention so I can avoid yet another review of the whole thing.

[Hans Wennborg]

(In reply to comment #21)
> (From update of attachment 63884 [details])
> WebKit/chromium/public/WebDeviceOrientation.h:49
>  +      static WebDeviceOrientation nullOrientation() { return WebDeviceOrientation(); }
> Since you're using (), C++ guarantees us that everything is 0'ed out?
No, WebDeviceOrientation() is the private constructor that sets m_isNull=true and leaves the rest uninitialized. I figured the values of the other members doesn't matter when m_isNull is true.

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
>  +      void setController(WebDeviceOrientationController*);
> Either virtual of WEBKIT_API
It is virtual since it's inherited from WebDeviceOrientationClient.

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
>  +      void initialize();
> WEBKIT_API
Done. (Also for reset()).

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:52
>  +      WebCore::DeviceOrientationClientMock* m_clientMock;
> I can't remember if this is allowed or not.  We might prefer a void* and casting...see if you can find other examples (like in PrivatePtr).
I can't find any classes in WebKit/chromium/public that have a void* member, and WebPrivatePtr<T> uses a T* member so I hope it's ok.

> WebKit/chromium/src/WebDeviceOrientation.cpp:70
>  +          return PassRefPtr<WebCore::DeviceOrientation>(0);
> Can't you just say "return 0;" ?
Done.

> WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
>  +      return WebDeviceOrientation(lastOrientation);
> Why store it to the temp variable...doesn't seem to give you much....and why do you need to cast it explicitly either?  It should just work.
Removing the temp variable.
I need to use the WebDeviceOrientation constructor explicitly. C++ will not see that it first needs to convert DeviceOrientation* to PassRefPtr<DeviceOrientation> which it can then use to construct a WebDeviceOrientation.

> 
> WebKit/chromium/src/WebViewImpl.cpp:290
>  +      if (WebRuntimeFeatures::isDeviceOrientationEnabled())
> Can't you do this either way?  Worst case, it gets set to null which is what would have happened anyway.
You're right. Done.

> Minor comments so r=me, but you're not a committer so you'll need to post another rev.  Let me know if you only address stuff I mention so I can avoid yet another review of the whole thing.


Uploading rebased patch addressing the stuff above (and nothing else).

[Hans Wennborg]

Created attachment 63896 [details]
Patch

[Satish Sampath]

WebKit/chromium/src/WebViewImpl.h:79
 +  class WebDeviceOrientationClient;
Is this required, as I don't see it used anywhere?

[Jeremy Orlow]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 63884 [details] [details])
> > WebKit/chromium/public/WebDeviceOrientation.h:49
> >  +      static WebDeviceOrientation nullOrientation() { return WebDeviceOrientation(); }
> > Since you're using (), C++ guarantees us that everything is 0'ed out?
> No, WebDeviceOrientation() is the private constructor that sets m_isNull=true and leaves the rest uninitialized. I figured the values of the other members doesn't matter when m_isNull is true.

Lets zero them out.  Or else at least add asserts that we're not null for every other accessor.
 
> > WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
> >  +      void setController(WebDeviceOrientationController*);
> > Either virtual of WEBKIT_API
> It is virtual since it's inherited from WebDeviceOrientationClient.

That makes ((WebDeviceOrientationClient *)new WebDeviceOrientationClientMock)->setController() work across the DLL barrier, but I believe not new WebDeviceOrientationClientMock->setController().
 
> > WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
> >  +      void initialize();
> > WEBKIT_API
> Done. (Also for reset()).
> 
> > WebKit/chromium/public/WebDeviceOrientationClientMock.h:52
> >  +      WebCore::DeviceOrientationClientMock* m_clientMock;
> > I can't remember if this is allowed or not.  We might prefer a void* and casting...see if you can find other examples (like in PrivatePtr).
> I can't find any classes in WebKit/chromium/public that have a void* member, and WebPrivatePtr<T> uses a T* member so I hope it's ok.

k

[Hans Wennborg]

(In reply to comment #24)
> WebKit/chromium/src/WebViewImpl.h:79
>  +  class WebDeviceOrientationClient;
> Is this required, as I don't see it used anywhere?
Thanks Satish, that was a left-over from a previous version. Removed.

(In reply to comment #25)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 63884 [details] [details] [details])
> > > WebKit/chromium/public/WebDeviceOrientation.h:49
> > >  +      static WebDeviceOrientation nullOrientation() { return WebDeviceOrientation(); }
> > > Since you're using (), C++ guarantees us that everything is 0'ed out?
> > No, WebDeviceOrientation() is the private constructor that sets m_isNull=true and leaves the rest uninitialized. I figured the values of the other members doesn't matter when m_isNull is true.
> 
> Lets zero them out.  Or else at least add asserts that we're not null for every other accessor.
Zeroing them out.

> 
> > > WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
> > >  +      void setController(WebDeviceOrientationController*);
> > > Either virtual of WEBKIT_API
> > It is virtual since it's inherited from WebDeviceOrientationClient.
> 
> That makes ((WebDeviceOrientationClient *)new WebDeviceOrientationClientMock)->setController() work across the DLL barrier, but I believe not new WebDeviceOrientationClientMock->setController().
Oh, I didn't realize that. Fixing.

> 
> > > WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
> > >  +      void initialize();
> > > WEBKIT_API
> > Done. (Also for reset()).
> > 
> > > WebKit/chromium/public/WebDeviceOrientationClientMock.h:52
> > >  +      WebCore::DeviceOrientationClientMock* m_clientMock;
> > > I can't remember if this is allowed or not.  We might prefer a void* and casting...see if you can find other examples (like in PrivatePtr).
> > I can't find any classes in WebKit/chromium/public that have a void* member, and WebPrivatePtr<T> uses a T* member so I hope it's ok.
> 
> k


Uploading new patch addressing the comments above.

[Hans Wennborg]

Created attachment 63904 [details]
Patch

[Jeremy Orlow]

Comment on attachment 63904 [details]
Patch

rubber stamp = me

[WebKit Commit Bot]

Comment on attachment 63904 [details]
Patch

Rejecting patch 63904 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--force']" exit_code: 1
Last 500 characters of output:
y.h
patching file WebKit/chromium/src/WebDeviceOrientation.cpp
patching file WebKit/chromium/src/WebDeviceOrientationClientMock.cpp
patching file WebKit/chromium/src/WebDeviceOrientationController.cpp
patching file WebKit/chromium/src/WebViewImpl.cpp
Hunk #2 FAILED at 178.
Hunk #3 succeeded at 266 (offset -1 lines).
Hunk #4 succeeded at 289 (offset -1 lines).
1 out of 4 hunks FAILED -- saving rejects to file WebKit/chromium/src/WebViewImpl.cpp.rej
patching file WebKit/chromium/src/WebViewImpl.h

Full output: http://queues.webkit.org/results/3722020

[Jeremy Orlow]

Comment on attachment 63904 [details]
Patch

Clearing flags on attachment: 63904

Committed r65063: <http://trac.webkit.org/changeset/65063>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.