54361 2011-02-13 11:29:47 -0800 [Chromium] Issue 50685: Move CC icons to chrome/common and implement loadPlatformResource in RendererWebKitClientImpl. 2011-02-28 13:40:48 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED WONTFIX P2 Normal --- 0 honten webkit-unassigned dhollowa fishd honten isherman jhawkins oldest_to_newest 350423 0 honten 2011-02-13 11:29:47 -0800 [Chromium] Issue 50685: Move CC icons to chrome/common and implement loadPlatformResource in RendererWebKitClientImpl. 350427 1 82271 honten 2011-02-13 11:32:32 -0800 Created attachment 82271 Patch 350429 2 honten 2011-02-13 11:35:49 -0800 Could you review? As I already mentioned in chrome CL, we have to commit at once. But the build itself file without chrome side commit. Just not working correctly. 352280 3 jhawkins 2011-02-16 12:58:42 -0800 +darin since I can't technically review. I'm not sure why you're adding loadPlatformResource() when there's already a loadResource() which does the same thing. When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource(). 352298 4 82271 fishd 2011-02-16 13:27:37 -0800 Comment on attachment 82271 Patch R- based on jhawkins feedback 353646 5 honten 2011-02-18 10:14:28 -0800 Thank you for your review!! 353663 6 honten 2011-02-18 10:42:13 -0800 Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name), And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now. Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685 That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl. Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)? What do you think? 353671 7 isherman 2011-02-18 10:48:48 -0800 (In reply to comment #6) > Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name), > And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now. > > Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685 > > That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl. > > Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)? > > What do you think? Let's see if we can just use Image::loadPlatformResource() for now, unless there's reason to go beyond that. 353690 8 honten 2011-02-18 11:12:34 -0800 Ilya, Thank you for your comment. I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name). Thanks, (In reply to comment #7) > (In reply to comment #6) > > Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name), > > And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now. > > > > Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685 > > > > That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl. > > > > Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)? > > > > What do you think? > > Let's see if we can just use Image::loadPlatformResource() for now, unless there's reason to go beyond that. 353692 9 isherman 2011-02-18 11:15:58 -0800 (In reply to comment #8) > I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name). What's wrong with calling PlatformBridge::loadPlatformImageResource()? 353715 10 honten 2011-02-18 11:46:16 -0800 IMHO, calling PlatformBridge::loadPlatformImageResource() is better. Because the function is also used from Source/WebCore/platform/graphics/chromium/ImageChromiumMac.mm And if we move it, we have to move ImageChromiumMac.mm also. That is another reason I changed PlatformBridge::loadPlatformImageResource(). (In reply to comment #9) > (In reply to comment #8) > > I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name). > > What's wrong with calling PlatformBridge::loadPlatformImageResource()? 353750 11 isherman 2011-02-18 12:34:03 -0800 To be clearer: Do we really need WebKit changes in order to move the resources in Chromium? If so, why? I really don't know this code at all well, but it doesn't seem to me like any WebKit changes should be necessary. 353798 12 honten 2011-02-18 13:29:29 -0800 Ilya, I agree we might not need to change WebKit if if we just move the resource. But, again, I already asked James here http://code.google.com/p/chromium/issues/detail?id=50685 >2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code? And he replied, >re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend. I don't know his intention, but I guess he wanted to separate the function according to the resource location. Thanks, (In reply to comment #11) > To be clearer: Do we really need WebKit changes in order to move the resources in Chromium? If so, why? > > I really don't know this code at all well, but it doesn't seem to me like any WebKit changes should be necessary. 353806 13 isherman 2011-02-18 13:45:25 -0800 (In reply to comment #12) > Ilya, > > I agree we might not need to change WebKit if if we just move the resource. > > > But, again, I already asked James here > > http://code.google.com/p/chromium/issues/detail?id=50685 > > >2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code? > > > And he replied, > > >re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend. > > I don't know his intention, but I guess he wanted to separate the function according to the resource location. It sounds like he meant Image::loadPlatformResource(): "When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource()." 353820 14 honten 2011-02-18 14:09:01 -0800 Ok, so do you agree to change Image::loadPlatformResource()? (In reply to comment #13) > (In reply to comment #12) > > Ilya, > > > > I agree we might not need to change WebKit if if we just move the resource. > > > > > > But, again, I already asked James here > > > > http://code.google.com/p/chromium/issues/detail?id=50685 > > > > >2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code? > > > > > > And he replied, > > > > >re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend. > > > > I don't know his intention, but I guess he wanted to separate the function according to the resource location. > > It sounds like he meant Image::loadPlatformResource(): > > "When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource()." 353826 15 isherman 2011-02-18 14:23:54 -0800 (In reply to comment #14) > Ok, so do you agree to change Image::loadPlatformResource()? Could you please explain what change exactly is needed and why? That might be easiest to do just by uploading the diff and perhaps pointing out the relevant part of it. Again, I do not know this corner of the code at all well, but it looks to me like we should be able to just use it as-is and still be able to move the image resources, which is the real goal. 353848 16 dhollowa 2011-02-18 14:48:21 -0800 It appears that: Image::loadPlatformResource calls PlatformBridge::loadPlatformImageResource which calls WebKitClient::loadResource So we end up at the same place via a different route. There's no advantage to adding a new method to the WebKitClient interface. There is little (if any) advantage to taking the Image::loadPlatformResource route. So I would lobby that we abandon this patch. However it would be very nice to rename the resources on the Chrome side to be chrome/common/resources/cc_amex.png, chrome/common/resources/cc_generic.png, etc. But let's discuss that in http://codereview.chromium.org/6516002/. (In reply to comment #15) > (In reply to comment #14) > > Ok, so do you agree to change Image::loadPlatformResource()? > > Could you please explain what change exactly is needed and why? That might be easiest to do just by uploading the diff and perhaps pointing out the relevant part of it. > > Again, I do not know this corner of the code at all well, but it looks to me like we should be able to just use it as-is and still be able to move the image resources, which is the real goal. 82271 2011-02-13 11:32:32 -0800 2011-02-16 13:27:37 -0800 Patch bug-54361-20110213113231.patch text/plain 2461 honten ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nIGIvU291cmNlL1dl YktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKaW5kZXggNmU5NTc4NWFiNjdiZGE4ZWYyY2E4YjUwNGIz ZjEyYjk5ZmVjM2QyOS4uYzJiNDQxNTJhYzgwNzBkZDJhODVhOTM1OTllN2U2YjVkOGFlOTJiYSAx MDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKKysrIGIvU291cmNl L1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMS0wMi0xMyAg TmFva2kgVGFrYW5vICA8dGFrYW5vLm5hb2tpQGdtYWlsLmNvbT4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbQ2hyb21pdW1dIElzc3VlIDUwNjg1OiBN b3ZlIENDIGljb25zIHRvIGNocm9tZS9jb21tb24gYW5kIGltcGxlbWVudCBsb2FkUGxhdGZvcm1S ZXNvdXJjZSBpbiBSZW5kZXJlcldlYktpdENsaWVudEltcGwuCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDM2MQorCisgICAgICAgICogcHVibGljL1dl YktpdENsaWVudC5oOgorICAgICAgICAoV2ViS2l0OjpXZWJLaXRDbGllbnQ6OmxvYWRQbGF0Zm9y bVJlc291cmNlKTogQWRkIHRoZSBuZXcgZnVuY3Rpb24uCisgICAgICAgICogc3JjL1BsYXRmb3Jt QnJpZGdlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBsYXRmb3JtQnJpZGdlOjpsb2FkUGxhdGZv cm1JbWFnZVJlc291cmNlKTogQ2hhbmdlIHRvIHVzZSBsb2FkUGxhdGZvcm1JbWFnZVJlc291cmNl KCkuCisKIDIwMTEtMDItMDkgIERhdmlkIEhvbGxvd2F5ICA8ZGhvbGxvd2FAY2hyb21pdW0ub3Jn PgogCiAgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEZpc2hlci4KZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYktpdENsaWVudC5oIGIvU291cmNlL1dlYktpdC9j aHJvbWl1bS9wdWJsaWMvV2ViS2l0Q2xpZW50LmgKaW5kZXggZDhlYWQyMzhlZDNiZDFlMDNiNjUz Nzg5YTllYzgwMmE3ODNlNDQ5MC4uZWFhOTA1ZTcwNjhmMDdiNGMyNTExYzA0YTU3ZjBlNWY1ZDVi MmZlNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJsaWMvV2ViS2l0Q2xp ZW50LmgKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJsaWMvV2ViS2l0Q2xpZW50LmgK QEAgLTIyMiw2ICsyMjIsOSBAQCBwdWJsaWM6CiAgICAgLy8gUmV0dXJucyBhIGJsb2Igb2YgZGF0 YSBjb3JyZXNwb25kaW5nIHRvIHRoZSBuYW1lZCByZXNvdXJjZS4KICAgICB2aXJ0dWFsIFdlYkRh dGEgbG9hZFJlc291cmNlKGNvbnN0IGNoYXIqIG5hbWUpIHsgcmV0dXJuIFdlYkRhdGEoKTsgfQog CisgICAgLy8gUmV0dXJucyBhIGJsb2Igb2YgZGF0YSBjb3JyZXNwb25kaW5nIHRvIHRoZSBuYW1l ZCBwbGF0Zm9ybSByZXNvdXJjZS4KKyAgICB2aXJ0dWFsIFdlYkRhdGEgbG9hZFBsYXRmb3JtUmVz b3VyY2UoY29uc3QgY2hhciogbmFtZSkgeyByZXR1cm4gV2ViRGF0YSgpOyB9CisKICAgICAvLyBE ZWNvZGVzIHRoZSBpbi1tZW1vcnkgYXVkaW8gZmlsZSBkYXRhIGFuZCByZXR1cm5zIHRoZSBsaW5l YXIgUENNIGF1ZGlvIGRhdGEgaW4gdGhlIGRlc3RpbmF0aW9uQnVzLgogICAgIC8vIEEgc2FtcGxl LXJhdGUgY29udmVyc2lvbiB0byBzYW1wbGVSYXRlIHdpbGwgb2NjdXIgaWYgdGhlIGZpbGUgZGF0 YSBpcyBhdCBhIGRpZmZlcmVudCBzYW1wbGUtcmF0ZS4KICAgICAvLyBSZXR1cm5zIHRydWUgb24g c3VjY2Vzcy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1BsYXRmb3Jt QnJpZGdlLmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1BsYXRmb3JtQnJpZGdlLmNw cAppbmRleCBiMDlkMTUwNjc0MGZhZDlkMjVkMzIwMjQxMDJkYTA1YmQ1ZGEwZDllLi5mNzkxY2Zk NTlhOWYyMzU4MTA4YmQ1ZWU4NGY0ZGI1NGIyOTlkZTRiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L2Nocm9taXVtL3NyYy9QbGF0Zm9ybUJyaWRnZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9j aHJvbWl1bS9zcmMvUGxhdGZvcm1CcmlkZ2UuY3BwCkBAIC02MjcsNyArNjI3LDcgQEAgTlBPYmpl Y3QqIFBsYXRmb3JtQnJpZGdlOjpwbHVnaW5TY3JpcHRhYmxlT2JqZWN0KFdpZGdldCogd2lkZ2V0 KQogCiBQYXNzUmVmUHRyPEltYWdlPiBQbGF0Zm9ybUJyaWRnZTo6bG9hZFBsYXRmb3JtSW1hZ2VS ZXNvdXJjZShjb25zdCBjaGFyKiBuYW1lKQogewotICAgIGNvbnN0IFdlYkRhdGEmIHJlc291cmNl ID0gd2ViS2l0Q2xpZW50KCktPmxvYWRSZXNvdXJjZShuYW1lKTsKKyAgICBjb25zdCBXZWJEYXRh JiByZXNvdXJjZSA9IHdlYktpdENsaWVudCgpLT5sb2FkUGxhdGZvcm1SZXNvdXJjZShuYW1lKTsK ICAgICBpZiAocmVzb3VyY2UuaXNFbXB0eSgpKQogICAgICAgICByZXR1cm4gSW1hZ2U6Om51bGxJ bWFnZSgpOwogCg==