Bug 133530

Summary: [iOS][WK2] Add device orientation
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, japhet, sam, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch thorton: review+

Description Benjamin Poulain 2014-06-04 18:59:37 PDT
[iOS][WK2] Add device orientation
Comment 1 Benjamin Poulain 2014-06-04 19:08:59 PDT
Created attachment 232517 [details]
Patch
Comment 2 Benjamin Poulain 2014-06-04 19:09:33 PDT
<rdar://problem/16680041>
Comment 3 WebKit Commit Bot 2014-06-04 19:11:24 PDT
Attachment 232517 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebFrame.mm:1273:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Benjamin Poulain 2014-06-04 20:19:31 PDT
Created attachment 232518 [details]
Patch
Comment 5 WebKit Commit Bot 2014-06-04 20:20:55 PDT
Attachment 232518 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebFrame.mm:1273:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Tim Horton 2014-06-05 13:27:55 PDT
Comment on attachment 232518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232518&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:193
> +        return 0;

sad that we don't have a comparable enum and use integer degrees instead, but not your fault

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:287
> +    if (deviceOrientation != m_deviceOrientation) {

This (and possibly those above) needs to check isValid, I think? (what happens if the WebProcess is dead when we get a UIWindowDidRotateNotification?)
Comment 7 Benjamin Poulain 2014-06-05 14:15:28 PDT
Committed r169625: <http://trac.webkit.org/changeset/169625>
Comment 8 Darin Adler 2014-06-05 15:54:39 PDT
Comment on attachment 232518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232518&action=review

> Source/WebCore/page/Frame.cpp:292
> +    for (Frame* frame = this; frame; frame = frame->tree().traverseNext())
> +        frames.append(*frame);

This is peculiar. It iterates starting with this frame, but continues up to this frames siblings and parents if the frame is not the main frame. I think this function is meant to be called only on the main frame, so if we do want it on the frame, then it should be on the MainFrame class.

But actually we try to avoid putting these directly into Frame these days, so normally we’d put this into something like EventHandler.

> Source/WebCore/page/Frame.cpp:297
> +    for (unsigned i = 0; i < frames.size(); i++) {
> +        if (Document* doc = frames[i]->document())
> +            doc->dispatchWindowEvent(Event::create(eventNames().orientationchangeEvent, false, false));
> +    }

This is old fashioned. Here’s the modern way to write it:

    for (auto& frame : frames) {
        if (Document* document = frame->document())
            document->dispatchWindowEvent(Event::create(eventNames().orientationchangeEvent, false, false));
    }