[chromium] make compositor requesting the font atlas #1
Created attachment 175473 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
commit order: https://bugs.webkit.org/show_bug.cgi?id=102958 https://codereview.chromium.org/11413123 https://bugs.webkit.org/show_bug.cgi?id=102960
Comment on attachment 175473 [details] Patch Attachment 175473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14960027
Created attachment 175486 [details] Patch
(In reply to comment #3) > commit order: > https://bugs.webkit.org/show_bug.cgi?id=102958 > https://codereview.chromium.org/11413123 > https://bugs.webkit.org/show_bug.cgi?id=102960 jamesr, can you please hava a look at them?
Comment on attachment 175486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175486&action=review > Source/Platform/ChangeLog:3 > + [chromium] make compositor request the font atlas #1 This does not describe the patch or the bug and is grammatically iffy. Please rephrase > Source/Platform/chromium/public/WebLayerTreeViewClient.h:109 > + virtual void createFontAtlas(SkBitmap&, WebRect[128], int&) { } The third parameter isn't very clear from the signature - could you provide a descriptive name?
(In reply to comment #7) > (From update of attachment 175486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175486&action=review > > > Source/Platform/ChangeLog:3 > > + [chromium] make compositor request the font atlas #1 > > This does not describe the patch or the bug and is grammatically iffy. Please rephrase Yeah, I found it pretty hard to find something descriptive. What would be better? - replace font atlas passing with requesting - make font atlas creation accessible to WebLayerTreeView and remove font atlas passing - replace WebLayerTreeView::setFontAtlas() with WebLayerTreeViewClient::createFontAtlas() I tried to find one name for all three patches and number them. Is that a good idea? I need some help here please. > > Source/Platform/chromium/public/WebLayerTreeViewClient.h:109 > > + virtual void createFontAtlas(SkBitmap&, WebRect[128], int&) { } > > The third parameter isn't very clear from the signature - could you provide a descriptive name? I had a look at how this was done before, the second parameter was named as well. Is this better?: virtual void createFontAtlas(SkBitmap&, WebRect asciiToRectTable[128], int& fontHeight) { } Is there a rule of thumb of when to name parameters in interface methods?
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 175486 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175486&action=review > > > > > Source/Platform/ChangeLog:3 > > > + [chromium] make compositor request the font atlas #1 > > > > This does not describe the patch or the bug and is grammatically iffy. Please rephrase > > Yeah, I found it pretty hard to find something descriptive. What would be better? > > - replace font atlas passing with requesting > - make font atlas creation accessible to WebLayerTreeView and remove font atlas passing > - replace WebLayerTreeView::setFontAtlas() with WebLayerTreeViewClient::createFontAtlas() > > I tried to find one name for all three patches and number them. Is that a good idea? I need some help here please. That sounds like a good list to have in the ChangeLog. For the top line, you could describe the API change this adds - for instance something like "[chromium] Add WebLayerTreeViewClient API to request font atlas" or something like that. > > > > Source/Platform/chromium/public/WebLayerTreeViewClient.h:109 > > > + virtual void createFontAtlas(SkBitmap&, WebRect[128], int&) { } > > > > The third parameter isn't very clear from the signature - could you provide a descriptive name? > > I had a look at how this was done before, the second parameter was named as well. Is this better?: > > virtual void createFontAtlas(SkBitmap&, WebRect asciiToRectTable[128], int& fontHeight) { } Yes > > Is there a rule of thumb of when to name parameters in interface methods? Yes - if the name of the parameter is fairly obvious from the type and signature, leave it out. Otherwise, include it. For instance having the first parameter be called 'bitmap' would be redundant with the type and calling it 'atlas' would be redundant with the function name, so leaving it out is fine. See http://www.webkit.org/coding/coding-style.html#names-verb
Created attachment 176048 [details] Patch
Comment on attachment 176048 [details] Patch Thanks, looks good.
Comment on attachment 176048 [details] Patch Rejecting attachment 176048 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: 0 (offset -10 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/Platform/chromium/public/WebLayerTreeViewClient.h.rej patching file Source/WebKit/chromium/src/WebViewImpl.cpp Hunk #1 succeeded at 4203 (offset -11 lines). patching file Source/WebKit/chromium/src/WebViewImpl.h Hunk #1 succeeded at 324 (offset -1 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/15018112
Created attachment 176341 [details] Patch
Comment on attachment 176341 [details] Patch Clearing flags on attachment: 176341 Committed r135929: <http://trac.webkit.org/changeset/135929>
All reviewed patches have been landed. Closing bug.