Created attachment 73109 [details] fix patch

Comment on attachment 73109 [details] fix patch Looks good! I still think we could pass Page* or Settings* as a parameter. Any specific reason for Frame?

(In reply to comment #2) > (From update of attachment 73109 [details]) > Looks good! I still think we could pass Page* or Settings* as a parameter. > > Any specific reason for Frame? The reason I prefer using Frame over Settings is that the caller doesn't have to do an additional check against pointer frame, which is taken care of inside the helper function. Design-wise, a user is asking if spatial navigation is enabled in a frame. The user shouldn't know about the internal implementation of Frame. In terms of Page, it's pretty much the same as Frame. Frame::settings() returns the page->settings(). In case its implementation changes for any reason, say, each frame has its own settings, this helper function needs no change.

Comment on attachment 73109 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=73109&action=review > WebCore/page/SpatialNavigation.cpp:58 > + if (frame && frame->settings() && frame->settings()->isSpatialNavigationEnabled()) > + return true; > + > + return false; This: if (x) return true; return false; Is the same as this: return x; You should remove the if.

sure! > This: > > if (x) > return true; > return false; > > Is the same as this: > > return x; > > You should remove the if.

Created attachment 73116 [details] fix patch 2

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 73109 [details] [details]) > > Looks good! I still think we could pass Page* or Settings* as a parameter. > > > > Any specific reason for Frame? > > The reason I prefer using Frame over Settings is that the caller doesn't have to do an additional check against pointer frame, which is taken care of inside the helper function. Design-wise, a user is asking if spatial navigation is enabled in a frame. The user shouldn't know about the internal implementation of Frame. > In terms of Page, it's pretty much the same as Frame. Frame::settings() returns the page->settings(). In case its implementation changes for any reason, say, each frame has its own settings, this helper function needs no change. I do not think there is any plan to make settings per Frame. It is essentially a Page concept.

The commit-queue encountered the following flaky tests while processing attachment 73116 [details]: http/tests/security/cross-origin-css.html Please file bugs against the tests. These tests were authored by cevans@google.com. The commit-queue is continuing to process your patch.

Comment on attachment 73116 [details] fix patch 2 Clearing flags on attachment: 73116 Committed r71479: <http://trac.webkit.org/changeset/71479>

All reviewed patches have been landed. Closing bug.