Set a max limit on extension items' labels in context menus....

Set a max limit on extension items' labels in context menus.

Also do some cleanup of context menu tests, including adding a generic
mechanism to let javascript pass a test message and have C++ test code wait
for and retrieve those messages.

BUG=49744
TEST=Follow steps in bug report


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54514

[rafaelw, 2010-07-30 21:20:37]

http://codereview.chromium.org/3017047/diff/1/4
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/3017047/diff/1/4#newcode90
chrome/browser/extensions/extension_menu_manager.cc:90: DCHECK(result.length()
<= max_length);
this seem just a tad defensive. is there some chance that TruncateString is
going to do the wrong thing here?

http://codereview.chromium.org/3017047/diff/1/8
File chrome/browser/extensions/extension_test_message_listener.cc (right):

http://codereview.chromium.org/3017047/diff/1/8#newcode32
chrome/browser/extensions/extension_test_message_listener.cc:32: NOTREACHED();
nit: DCHECK(!empty) and skip the if clause. also, are you sure we always run
browser tests with debug builds? it'd be a bummer to return an empty result
here.

http://codereview.chromium.org/3017047/diff/1/9
File chrome/browser/extensions/extension_test_message_listener.h (right):

http://codereview.chromium.org/3017047/diff/1/9#newcode31
chrome/browser/extensions/extension_test_message_listener.h:31: //
InProcessBrowserTest::CleanUpOnMainThread() method and clean it up there.
Can I suggest a few things regarding this class:

1) Rather than expose a queue/wait interface, why not expose a listener/wait-all
interface. i.e.

ExtensionTestMessageListener listener("message 1"); // starts listening
immediately for the notification & this message)
listener.WaitForMessage("message 2"); // optional

.. Do work

listener.WaitAll(); // exits when all added messages have been received.

the implementation can be just an internal set of string messages that are yet
to be received. WaitAll() runs the message loop if it is non-empty. Receiving a
notification removes the (possibly non-present message) from the set and checks
to see if empty.

2) Consider putting it in ui_test_utils.cc

http://codereview.chromium.org/3017047/diff/1/11
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/3017047/diff/1/11#newcode35
chrome/browser/tab_contents/render_view_context_menu.h:35: static const size_t
kMaxExtensionItemTitleLength;
why is one of these size_t and the other int?

[asargent_no_longer_on_chrome, 2010-07-30 22:34:28]

http://codereview.chromium.org/3017047/diff/1/4
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/3017047/diff/1/4#newcode90
chrome/browser/extensions/extension_menu_manager.cc:90: DCHECK(result.length()
<= max_length);
On 2010/07/30 21:20:37, rafaelw wrote:
> this seem just a tad defensive. is there some chance that TruncateString is
> going to do the wrong thing here?

I had an earlier bug in my test where the check on the label size at the end of
my test failed (it was 2 characters too long), and had put this DCHECK here to
help me understand if TruncateString was messing up. It turns out the problem
was that I was checking against a result that had been converted to a
std::string and the unicode elipsis character that TruncateString put in took up
3 bytes in UTF-8.

Anyhow, I've removed it.

http://codereview.chromium.org/3017047/diff/1/8
File chrome/browser/extensions/extension_test_message_listener.cc (right):

http://codereview.chromium.org/3017047/diff/1/8#newcode32
chrome/browser/extensions/extension_test_message_listener.cc:32: NOTREACHED();
On 2010/07/30 21:20:37, rafaelw wrote:
> nit: DCHECK(!empty) and skip the if clause. also, are you sure we always run
> browser tests with debug builds? it'd be a bummer to return an empty result
> here.

Switched to CHECK(!empty) which will fail in release and debug.

http://codereview.chromium.org/3017047/diff/1/9
File chrome/browser/extensions/extension_test_message_listener.h (right):

http://codereview.chromium.org/3017047/diff/1/9#newcode31
chrome/browser/extensions/extension_test_message_listener.h:31: //
InProcessBrowserTest::CleanUpOnMainThread() method and clean it up there.
> 1) Rather than expose a queue/wait interface, why not expose a
listener/wait-all
> interface. i.e.
> 
> ExtensionTestMessageListener listener("message 1"); // starts listening
> immediately for the notification & this message)
> listener.WaitForMessage("message 2"); // optional
> 
> .. Do work
> 
> listener.WaitAll(); // exits when all added messages have been received.


2 problems with your suggested approach:
-I was thinking it could be handy in the future to be able to pass up some data
from javascript to C++. A simple example might be something like a tab event
listener sending the tabid it just saw an event for, so that we can check in C++
that it was the right tabid.

-I explicitly wanted to decouple the start of listening for messages before a
blocking wait to help avoid races. Consider this example:

LoadExtension(...)
ExtensionTestMessageListener listener("whatever");


Start loading the extension, now we're running the message loop inside
LoadExtension and we have this situation:

message loop pending tasks:
1 IPC that ends up generating an EXTENSION_LOADED notification
2 IPC that ends up generating an EXTENSION_TEST_MESSAGE notification

When we process task 1, LoadExtension's Observer ends up posting a Quit task.
Now we're in this situation:

message loop pending tasks:
2 IPC that ends up generating an EXTENSION_TEST_MESSAGE notification
3 Quit task

Oops! No one is listening for the EXTENSION_TEST_MESSAGE when it's Notify'd,
then we exit the message loop and return from LoadExtension. Now
ExtensionTestMessageListener doesn't see any message and the test times out.



> 2) Consider putting it in ui_test_utils.cc

Isn't this specific to extensions browser tests, and therefore wouldn't be used
by non-extenion ui tests?

http://codereview.chromium.org/3017047/diff/1/11
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/3017047/diff/1/11#newcode35
chrome/browser/tab_contents/render_view_context_menu.h:35: static const size_t
kMaxExtensionItemTitleLength;
On 2010/07/30 21:20:37, rafaelw wrote:
> why is one of these size_t and the other int?

Good catch - switched the second one to size_t.

[asargent_no_longer_on_chrome, 2010-07-30 23:36:19]

Changed the ExtensionTestListener class based on our offline discussion. Take
another look.

[rafaelw, 2010-07-31 00:03:33]

lgtm. nice!

http://codereview.chromium.org/3017047/diff/25001/5008
File chrome/browser/extensions/extension_test_message_listener.cc (right):

http://codereview.chromium.org/3017047/diff/25001/5008#newcode27
chrome/browser/extensions/extension_test_message_listener.cc:27: return
satisfied_;
nit: DCHECK(satisfied);

http://codereview.chromium.org/3017047/diff/25001/5008#newcode36
chrome/browser/extensions/extension_test_message_listener.cc:36: satisfied_ =
true;
might also be worth removing the listen here. just in case a test has multiple
and we end up tracking down the notifications.