Bug 102958 – [chromium] Add WebLayerTreeViewClient API to request font atlas

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] make compositor requesting the font atlas #1

Created attachment 175473 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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

[WebKit Review Bot]

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?

[James Robinson]

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?

[James Robinson]

(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

[James Robinson]

Comment on attachment 176048 [details]
Patch

Thanks, looks good.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 176341 [details]
Patch

Clearing flags on attachment: 176341

Committed r135929: <http://trac.webkit.org/changeset/135929>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.