Bug 133530 - [iOS][WK2] Add device orientation
Summary: [iOS][WK2] Add device orientation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-04 18:59 PDT by Benjamin Poulain
Modified: 2014-06-05 15:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (34.30 KB, patch)
2014-06-04 19:08 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (34.32 KB, patch)
2014-06-04 20:19 PDT, Benjamin Poulain
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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));
    }