Bug 91541 - [BlackBerry] Move about: URL handling out of WebCore
Summary: [BlackBerry] Move about: URL handling out of WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-17 14:05 PDT by Yong Li
Modified: 2012-07-23 15:34 PDT (History)
5 users (show)

See Also:


Attachments
the patch (57.46 KB, patch)
2012-07-17 14:36 PDT, Yong Li
no flags Details | Formatted Diff | Diff
try again (57.43 KB, patch)
2012-07-18 09:42 PDT, Yong Li
no flags Details | Formatted Diff | Diff
move to a new place (16.01 KB, patch)
2012-07-23 11:56 PDT, Yong Li
no flags Details | Formatted Diff | Diff
fix style (16.00 KB, patch)
2012-07-23 12:00 PDT, Yong Li
rwlbuis: review-
Details | Formatted Diff | Diff
More cleanups (16.59 KB, patch)
2012-07-23 13:39 PDT, Yong Li
rwlbuis: review+
Details | Formatted Diff | Diff
fix style (16.57 KB, patch)
2012-07-23 14:05 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-07-17 14:05:09 PDT
To move about: URL handling  from network layer to WebKit/blackberry
Comment 1 Yong Li 2012-07-17 14:36:07 PDT
Created attachment 152835 [details]
the patch
Comment 2 WebKit Review Bot 2012-07-17 14:40:27 PDT
Attachment 152835 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:20:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yong Li 2012-07-18 09:42:45 PDT
Created attachment 153029 [details]
try again
Comment 4 Rob Buis 2012-07-18 09:57:59 PDT
Comment on attachment 153029 [details]
try again

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

Looks good.

> Source/WebKit/blackberry/Api/WebPage_p.h:100
> +    bool loadAbout(const char* aboutURL);

Could it be const?
Comment 5 WebKit Review Bot 2012-07-18 11:25:19 PDT
Comment on attachment 153029 [details]
try again

Clearing flags on attachment: 153029

Committed r122992: <http://trac.webkit.org/changeset/122992>
Comment 6 WebKit Review Bot 2012-07-18 11:25:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Yong Li 2012-07-23 09:38:51 PDT
should be moved to the right place
Comment 8 Yong Li 2012-07-23 11:56:04 PDT
Created attachment 153824 [details]
move to a new place
Comment 9 WebKit Review Bot 2012-07-23 11:57:35 PDT
Attachment 153824 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:359:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Yong Li 2012-07-23 12:00:37 PDT
Created attachment 153828 [details]
fix style
Comment 11 Rob Buis 2012-07-23 12:37:58 PDT
Comment on attachment 153828 [details]
fix style

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

Looks good, still some issues.

> Source/WebKit/blackberry/ChangeLog:11
> +        Other changes: Remove about:version which makes little sense. Make about:memory partially visible.

Bu about:version is still in use.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:397
> +            WTF::RefPtr<SharedBuffer> buffer = SharedBuffer::create(source.is8Bit() ? reinterpret_cast<const char*>(source.characters8()) : source.latin1().data(), source.length());

No WTF:: prefix needed.

> Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:328
> +    } else if (equalIgnoringCase(aboutWhat, "cache")) {

We seem to be dealing with cache a lot, how about an if block for when aboutWhat starts with "cache" and then do the subtests?

> Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:344
> +        result.append(String("<html><head><title>BlackBerry Browser Disk Cache</title></head><body>Http disk cache is enabled.</body></html>"));

These two blocks are so similar that maybe a helper function can be added for them.
Comment 12 Yong Li 2012-07-23 13:39:01 PDT
Created attachment 153852 [details]
More cleanups
Comment 13 WebKit Review Bot 2012-07-23 13:42:10 PDT
Attachment 153852 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:394:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Rob Buis 2012-07-23 13:58:51 PDT
Comment on attachment 153852 [details]
More cleanups

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

Looks good.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:397
> +            WTF::RefPtr<SharedBuffer> buffer = SharedBuffer::create(source.is8Bit() ? reinterpret_cast<const char*>(source.characters8()) : source.latin1().data(), source.length());

You forgot to remove the WTF:: prefix :)
Comment 15 Yong Li 2012-07-23 14:05:50 PDT
Created attachment 153858 [details]
fix style
Comment 16 WebKit Review Bot 2012-07-23 15:34:30 PDT
Comment on attachment 153858 [details]
fix style

Clearing flags on attachment: 153858

Committed r123391: <http://trac.webkit.org/changeset/123391>
Comment 17 WebKit Review Bot 2012-07-23 15:34:36 PDT
All reviewed patches have been landed.  Closing bug.