Bug 78517

Summary: [BlackBerry] Upstream backing store related classes
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: PlatformAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: jkjiang, leo.yang, levin+threading, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch tonikitoo: review+, tonikitoo: commit-queue-

Description Rob Buis 2012-02-13 12:18:41 PST
This consists of 6 files.
Comment 1 Rob Buis 2012-02-13 12:21:58 PST
Created attachment 126805 [details]
Patch
Comment 2 Antonio Gomes 2012-02-13 12:24:49 PST
Comment on attachment 126805 [details]
Patch

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

> Source/WebKit/blackberry/WebKitSupport/BackingStoreCompositingSurface.cpp:97
> +#if OS(QNX)

probably unneeded?

> Source/WebKit/blackberry/WebKitSupport/BackingStoreCompositingSurface.h:2
> + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights reserved.

2012?

> Source/WebKit/blackberry/WebKitSupport/BackingStoreTile.cpp:145
> +#if OS(QNX)

ditto
Comment 3 Rob Buis 2012-02-13 12:33:51 PST
Created attachment 126806 [details]
Patch
Comment 4 Rob Buis 2012-02-14 07:03:37 PST
Created attachment 126972 [details]
Patch
Comment 5 Rob Buis 2012-02-14 07:05:29 PST
(In reply to comment #4)
> Created an attachment (id=126972) [details]
> Patch

This latest patch contains Leo's recent changes to BackingStoreCompositingSurface. I did similar changes to BackingStoreTile, and also added a cleaned up version of BackingStoreClient. Of course this stuff compiles :)
Cheers,

Rob.
Comment 6 WebKit Review Bot 2012-02-14 07:10:03 PST
Attachment 126972 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Antonio Gomes 2012-02-14 07:15:23 PST
Comment on attachment 126972 [details]
Patch

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

one more round needed.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:43
> +static inline WebCore::IntSize pointToSize(const WebCore::IntPoint& point)

unneeded webcore:: here and elsewhere in the patch.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:144
> +    if (!isMainFrame()) {
> +        // It is possible that the owner HTML element has been removed at this point,
> +        // especially when the frame is loading a JavaScript URL.
> +        if (Element* elt = m_frame->ownerElement()) {
> +            if (RenderBox* obj = elt->renderBox())
> +                rect.move(obj->borderLeft() + obj->paddingLeft(), obj->borderTop() + obj->paddingTop());
> +        }
> +    }
> +
> +    WebCore::Frame* frame = m_frame;
> +    while (frame) {
> +        if (Element* element = static_cast<Element*>(frame->ownerElement())) {
> +            do {
> +                rect.move(element->offsetLeft(), element->offsetTop());
> +            } while ((element = element->offsetParent()));
> +        }
> +
> +        if ((frame = frame->tree()->parent()))
> +            rect.move((-frame->view()->scrollOffset()));
> +    }

we can certainly improve this/speed it up, but in a follow up. I think I had done that already.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.h:93
> +    bool isLoading() const;
> +    WebPagePrivate::LoadState loadState() const;

these methods should not belong here, but that is another story...

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.h:115
> +    // FIXME: Leaving the below lines commented out as a reference for us to soon be sure if we need these
> +    // methods and class variables be moved from WebPage to BackingStoreClient.
> +    // Notification methods that deliver changes to the real geometry of the device as specified above.
> +    // void notifyTransformChanged();
> +    // void notifyTransformedContentsSizeChanged();
> +    // void notifyTransformedScrollChanged();
> +    // m_overflowExceedsContentsSize = true;

hummmmm....

> Source/WebKit/blackberry/WebKitSupport/BackingStoreCompositingSurface.h:21
> +#ifndef BackingStoreCompositingSurface_h
> +#define BackingStoreCompositingSurface_h
> +

is this the same as leo's patch? with the internal changes/clean ups he did yesterday?
Comment 8 Rob Buis 2012-02-14 08:40:30 PST
Created attachment 126985 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-14 08:42:51 PST
Attachment 126985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Antonio Gomes 2012-02-14 11:00:51 PST
Comment on attachment 126985 [details]
Patch

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

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:31
> +#include "IntPoint.h"
> +#include "IntRect.h"
> +#include "IntSize.h"

do we need to include all these?

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:364
> +    return m_frame
> +        && m_frame->page()
> +        && m_frame->page()->focusController()
> +        && m_frame->page()->focusController()->focusedFrame() == m_frame;

wrong indentation style I think.
Comment 11 Rob Buis 2012-02-14 11:14:22 PST
Landed in r107716.
Comment 12 Leo Yang 2012-02-14 17:32:11 PST
*** Bug 78592 has been marked as a duplicate of this bug. ***
Comment 13 Jacky Jiang 2012-02-29 12:09:48 PST
*** Bug 79919 has been marked as a duplicate of this bug. ***