| Summary: | [Chromium] Call setToolTipText() in WebPopupMenuImpl mouse move handler to show tool tip in select popup window. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||
| Component: | New Bugs | Assignee: | Abhishek Arya <inferno> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, dglazkov, eric, fishd, honten, inferno, tkent | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Naoki Takano
2011-05-22 22:45:49 PDT
Created attachment 94369 [details]
Patch
Comment on attachment 94369 [details]
Patch
Please review.
Comment on attachment 94369 [details]
Patch
Does this patch work with all Chromium platforms? I'm afraid for Mac.
Ah, you are right, I have to look into more Mac... But I don't think the patch harms Mac code like crash. But how about Win and Linux? Is the direction is right? Especially I'm a bit afraid of static_cast<PopupContainer*>(), but it looks Ok after I analyzed statically with code search. Thanks, (In reply to comment #3) > (From update of attachment 94369 [details]) > Does this patch work with all Chromium platforms? I'm afraid for Mac. (In reply to comment #4) > Ah, you are right, I have to look into more Mac... But I don't think the patch harms Mac code like crash. > > But how about Win and Linux? Is the direction is right? Especially I'm a bit afraid of static_cast<PopupContainer*>(), but it looks Ok after I analyzed statically with code search. The patch would be a right direction if we ignored Mac. However we should have a solution for Mac, and the solution might be applied to all platforms. So I don't accept this patch at this moment. Sure, I try to fix also on Mac. My Mac is really slow though... (In reply to comment #5) > (In reply to comment #4) > > Ah, you are right, I have to look into more Mac... But I don't think the patch harms Mac code like crash. > > > > But how about Win and Linux? Is the direction is right? Especially I'm a bit afraid of static_cast<PopupContainer*>(), but it looks Ok after I analyzed statically with code search. > > The patch would be a right direction if we ignored Mac. However we should have a solution for Mac, and the solution might be applied to all platforms. So I don't accept this patch at this moment. Right now, I'm looking into Mac side source code. And I noticed very interesting thing. The change for Mac is completely independent from Linux and Win change. As you know, Mac uses PopupMenuHelper::ShowPopupMenu() to show popup window, and we can pass the whole tool tip text at once right before the window pops up. Right now, I'm writing the code and I will be able to upload tomorrow. Thanks, (In reply to comment #6) > Sure, I try to fix also on Mac. > > My Mac is really slow though... > > (In reply to comment #5) > > (In reply to comment #4) > > > Ah, you are right, I have to look into more Mac... But I don't think the patch harms Mac code like crash. > > > > > > But how about Win and Linux? Is the direction is right? Especially I'm a bit afraid of static_cast<PopupContainer*>(), but it looks Ok after I analyzed statically with code search. > > > > The patch would be a right direction if we ignored Mac. However we should have a solution for Mac, and the solution might be applied to all platforms. So I don't accept this patch at this moment. Created attachment 94730 [details]
Patch
Comment on attachment 94730 [details]
Patch
Please review.
It includes Mac change.
Could you review? As you know, Eric or Kent-san, Could you review my patch? I already got LGTM in Mac part. Thanks, I'm not very useful for chromium WebKit reviews. WebCore does have manual tests. You could add a manual test which allowed us to test across all webcore implementations that we do RTL tooltips correctly, etc. Comment on attachment 94730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94730&action=review > Source/WebKit/chromium/public/WebWidgetClient.h:86 > + virtual void setToolTipText(const WebString&, WebTextDirection hint) { } be sure to cleanup the chromium side so that the methods are declared in the proper order in render_view.{h,cc}. Thank you for your review, fishd. Here is the chromium side patch, http://codereview.chromium.org/6974007/ If you have time, could you review it too? Also we need to commit this patch at the same time with chromium and webkit. How I can commit them at the same time? BTW, I cannot find you e-mail address, fishd@chromium.org in Chromium CL. Do you have another e-mail? Thanks, (In reply to comment #13) > (From update of attachment 94730 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94730&action=review > > > Source/WebKit/chromium/public/WebWidgetClient.h:86 > > + virtual void setToolTipText(const WebString&, WebTextDirection hint) { } > > be sure to cleanup the chromium side so that the methods are declared in the > proper order in render_view.{h,cc}. (In reply to comment #14) > Thank you for your review, fishd. > > Here is the chromium side patch, > http://codereview.chromium.org/6974007/ > If you have time, could you review it too? > > Also we need to commit this patch at the same time with chromium and webkit. > How I can commit them at the same time? It is really necessary in this case? It looks like render_view.cc should still compile and work properly if only this WebKit change lands. If I'm wrong, then what we sometimes do is add a #define to a WebKit API header. Then, we land a Chromium patch that is conditional on that #define to implement either the old API or the new API. So land that Chromium patch first. Then land the WebKit patch. Then land a Chromium patch to clean out the conditional code. Then go back and land another WebKit patch to remove the #define. Lot's of back-n-forth, but it is fairly easy once you get used to it. Not sure it is necessary in this case though. > BTW, I cannot find you e-mail address, fishd@chromium.org in Chromium CL. > Do you have another e-mail? Use darin@chromium.org. Sorry for the confusion. I only use the fishd alias on WebKit to avoid confusion with Darin Adler. Bugzilla has a habit of only showing usernames, dropping the @domain part :-P I couldn't build correctly at the last time because of virtual derivation. But I'll check it again. Once it looks Ok, I'll add manual-test and then ask again. Thanks, (In reply to comment #15) > (In reply to comment #14) > > Thank you for your review, fishd. > > > > Here is the chromium side patch, > > http://codereview.chromium.org/6974007/ > > If you have time, could you review it too? > > > > Also we need to commit this patch at the same time with chromium and webkit. > > How I can commit them at the same time? > > It is really necessary in this case? It looks like render_view.cc should still compile and work properly if only this WebKit change lands. If I'm wrong, then what we sometimes do is add a #define to a WebKit API header. Then, we land a Chromium patch that is conditional on that #define to implement either the old API or the new API. So land that Chromium patch first. Then land the WebKit patch. Then land a Chromium patch to clean out the conditional code. Then go back and land another WebKit patch to remove the #define. Lot's of back-n-forth, but it is fairly easy once you get used to it. Not sure it is necessary in this case though. > > > > BTW, I cannot find you e-mail address, fishd@chromium.org in Chromium CL. > > Do you have another e-mail? > > Use darin@chromium.org. Sorry for the confusion. I only use the fishd alias on WebKit to avoid confusion with Darin Adler. Bugzilla has a habit of only showing usernames, dropping the @domain part :-P Created attachment 95245 [details]
Patch
Comment on attachment 95245 [details]
Patch
Added manual test.
Also I made sure we don't have to land the patch at once btw Chromium and WebKit.
So please land this patch.
Darin, Could you review again? As I wrote here, I just added manual test. Thanks, (In reply to comment #18) > (From update of attachment 95245 [details]) > Added manual test. > > Also I made sure we don't have to land the patch at once btw Chromium and WebKit. > > So please land this patch. Can anybody review my patch? The main c++ part review is already done. Just I want to make sure manual test is Ok or not. Thanks, (In reply to comment #19) > Darin, > > Could you review again? > > As I wrote here, I just added manual test. > > Thanks, > > (In reply to comment #18) > > (From update of attachment 95245 [details] [details]) > > Added manual test. > > > > Also I made sure we don't have to land the patch at once btw Chromium and WebKit. > > > > So please land this patch. Does anybody review my patch? Chromium side review is done, but I cannot commit without this patch landing. (In reply to comment #20) > Can anybody review my patch? > > The main c++ part review is already done. > > Just I want to make sure manual test is Ok or not. > > Thanks, > > (In reply to comment #19) > > Darin, > > > > Could you review again? > > > > As I wrote here, I just added manual test. > > > > Thanks, > > > > (In reply to comment #18) > > > (From update of attachment 95245 [details] [details] [details]) > > > Added manual test. > > > > > > Also I made sure we don't have to land the patch at once btw Chromium and WebKit. > > > > > > So please land this patch. Eric, Could you land my patch? As you see, I just added manual test here as you suggested me. So I believe you can review it. Thanks, (In reply to comment #21) > Does anybody review my patch? > > Chromium side review is done, but I cannot commit without this patch landing. > > (In reply to comment #20) > > Can anybody review my patch? > > > > The main c++ part review is already done. > > > > Just I want to make sure manual test is Ok or not. > > > > Thanks, > > > > (In reply to comment #19) > > > Darin, > > > > > > Could you review again? > > > > > > As I wrote here, I just added manual test. > > > > > > Thanks, > > > > > > (In reply to comment #18) > > > > (From update of attachment 95245 [details] [details] [details] [details]) > > > > Added manual test. > > > > > > > > Also I made sure we don't have to land the patch at once btw Chromium and WebKit. > > > > > > > > So please land this patch. Comment on attachment 95245 [details]
Patch
rs=me based on fishd's previous r+.
Comment on attachment 95245 [details] Patch Clearing flags on attachment: 95245 Committed r88021: <http://trac.webkit.org/changeset/88021> All reviewed patches have been landed. Closing bug. The commit-queue encountered the following flaky tests while processing attachment 95245 [details]: http/tests/websocket/tests/frame-length-longer-than-buffer.html bug 61837 (author: abarth@webkit.org) The commit-queue is continuing to process your patch. Created attachment 96149 [details]
Ignore this.
Sorry wrong bug. |