Bug 73612

Summary: Upstream about: feature in WebKit/blackberry/WebCoreSupport/
Product: WebKit Reporter: Leo Yang <leo.yang>
Component: PlatformAssignee: Leo Yang <leo.yang>
Status: RESOLVED FIXED    
Severity: Normal CC: charles.wei, dbates, rwlbuis, staikos, tonikitoo, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
Patch
eric: review-
Patch v2
none
Patch v3
rwlbuis: review-
Patch v4 tonikitoo: review+

Leo Yang
Reported 2011-12-01 19:22:17 PST
This is the BlackBerry platform specific about data.
Attachments
Patch (44.39 KB, patch)
2011-12-01 19:26 PST, Leo Yang 		eric: review-
DetailsFormatted DiffDiff
Patch v2 (47.14 KB, patch)
2011-12-04 18:23 PST, Leo Yang 		no flags
DetailsFormatted DiffDiff
Patch v3 (47.39 KB, patch)
2011-12-04 23:27 PST, Leo Yang 		rwlbuis: review-
DetailsFormatted DiffDiff
Patch v4 (26.14 KB, patch)
2011-12-06 00:37 PST, Leo Yang 		tonikitoo: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Leo Yang
Comment 1 2011-12-01 19:26:29 PST
Created attachment 117545 [details] Patch
Rob Buis
Comment 2 2011-12-02 06:46:56 PST
This looks good. But maybe it can be cleaned up some more, some features got removed, see for instance: https://bugs.webkit.org/show_bug.cgi?id=68018 (ENABLE_SVG_FOREIGN_OBJECT)
Eric Seidel (no email)
Comment 3 2011-12-03 13:33:13 PST
Comment on attachment 117545 [details] Patch Do other ports do this? I'm not sure this information really belongs in WebCore. It seems like embedder-level functionality.
Leo Yang
Comment 4 2011-12-04 18:15:42 PST
Let me move to WebKit/blackberry/WebCoreSupport/
Leo Yang
Comment 5 2011-12-04 18:16:38 PST
(In reply to comment #2) > This looks good. But maybe it can be cleaned up some more, some features got removed, see for instance: > > https://bugs.webkit.org/show_bug.cgi?id=68018 (ENABLE_SVG_FOREIGN_OBJECT) Thanks. But can I do this in a separated bug? After they are in, I'll file a bug to track up-to-date issue.
Leo Yang
Comment 6 2011-12-04 18:23:42 PST
Created attachment 117820 [details] Patch v2
Antonio Gomes
Comment 7 2011-12-04 20:33:13 PST
(In reply to comment #5) > (In reply to comment #2) > > This looks good. But maybe it can be cleaned up some more, some features got removed, see for instance: > > > > https://bugs.webkit.org/show_bug.cgi?id=68018 (ENABLE_SVG_FOREIGN_OBJECT) > > Thanks. But can I do this in a separated bug? After they are in, I'll file a bug to track up-to-date issue. It is hard to say, Leo. I know our last rebase is behind this macro removals/clean up, but reintroducing them does not sounds right to me.
Leo Yang
Comment 8 2011-12-04 23:27:14 PST
Created attachment 117849 [details] Patch v3 OK. Let me up it to date.
Rob Buis
Comment 9 2011-12-05 06:55:02 PST
Comment on attachment 117849 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=117849&action=review I think another round is needed, to cleanup the code some more. Good job on figuring out what Features got dropped and added. > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2126 > + "YARR_JIT_DEBUG"); The features seem to match now AFAICS. But looking at this repeated structure, this seems to cry out for a macro :) > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2143 > +static const int s_kilobyte = 1024; Should use DEFINE_STATIC_LOCAL within cacheTypeStatisticToHTMLTr > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2153 > + return tr; Maybe does not need the temp var.
Leo Yang
Comment 10 2011-12-06 00:36:15 PST
(In reply to comment #9) > (From update of attachment 117849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117849&action=review > > I think another round is needed, to cleanup the code some more. Good job on figuring out what Features got dropped and added. > > > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2126 > > + "YARR_JIT_DEBUG"); > > The features seem to match now AFAICS. But looking at this repeated structure, this seems to cry out for a macro :) > C++ doesn't support pre-processing instructions in a macro. I'll use a perl script to generate code fragments. > > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2143 > > +static const int s_kilobyte = 1024; > > Should use DEFINE_STATIC_LOCAL within cacheTypeStatisticToHTMLTr Actually static is not needed. I'll change to const int s_kiloByte = 1024 and put it in the cacheTypeStatisticToHTMLTr. > > > Source/WebKit/blackberry/WebCoreSupport/AboutData.cpp:2153 > > + return tr; > > Maybe does not need the temp var. OK.
Leo Yang
Comment 11 2011-12-06 00:37:51 PST
Created attachment 118003 [details] Patch v4 Using perl script to generate code fragments.
Leo Yang
Comment 12 2011-12-06 18:37:53 PST
Committed r102207: <http://trac.webkit.org/changeset/102207>
Note You need to log in before you can comment on or make changes to this bug.