Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
865373002:
Removed calls to [HyperlinkTextView setMessageAndLink:withLink:...] (Closed)
|
Created:
5 years, 11 months ago by shrike Modified:
5 years, 10 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, tfarina, noyau+watch_chromium.org, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoved calls to [HyperlinkTextView setMessageAndLink:withLink:...]
Replaced occurrences of
[HyperlinkTextView setMessageAndLink:withLink:atOffset:font:
messageColor:linkColor:]
with
[HyperlinkTextView setMessage:withFont:messageColor:] +
[HyperlinkTextView addLinkRange:withName:linkColor]
BUG=253755
TEST=for bookmark_sync_promo_controller.mm: Launch the Chrome browser. Click the
star icon at the right end of the omni box. In the popover window that appears,
confirm that the text at the bottom reads, “Sign in to get your bookmarks
everywhere.” and that the text “Sign in” is a clickable link that takes you to
the Chrome sign-in page.
for extension_installed_bubble_controller.mm: Launch the Chrome browser and type
chrome://extensions in the omnibox. In the page that appears click the Get more
extensions link at the bottom. Locate the Tag Assistant by Google extension and
click it. Click the “+ FREE” button in the upper-right corner of the overlay
window that appears. Click “Add” in the overlay window that appears to confirm
your action. In the bubble window that appears that says Tag Assistant has been
added to Chrome, confirm that the text at the bottom of the bubble window says,
“Sign in to Chrome to get this extension, your history, and other Chrome
settings on all your devices.”, and that “Sign in to Chrome” is a clickable link
that takes you to the Chrome sign-in page.
for exclusive_access_bubble_window_controller.mm: Launch the Chrome browser and
go to http://davidwalsh.name/demo/fullscreen.php . Click the “Launch Fullscreen”
button that appears halfway down the page. If a bubble window appears stating,
“davidwalsh.name is now full screen” click Allow. Confirm that in the next
bubble window that appears, the text at the right says, “Exit full screen (Esc)”
and that the words “Exit full screen” are a clickable link that exits full
screen mode when clicked.
Committed: https://crrev.com/f9bf8a81ff2ddd329846b2006185223799bc6bfb
Cr-Commit-Position: refs/heads/master@{#313774}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed unnecessary checks, changed declaration #
Total comments: 6
Patch Set 3 : Removed unnecessary check, changed formatting #
Total comments: 5
Patch Set 4 : Changed line wrapping #Patch Set 5 : Minor change to address merge conflict #
Messages
Total messages: 40 (6 generated)
shrike@chromium.org changed reviewers: + asvitkine@chromium.org
Thanks for reviewing my changes - let me know if you have feedback.
https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm (right): https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:447: if ([link length] != 0) { When would the link length be 0? https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:453: if ([link length] != 0) { Ditto.
On 2015/01/23 18:41:02, Alexei Svitkine wrote: > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... > File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm > (right): > > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... > chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:447: > if ([link length] != 0) { > When would the link length be 0? > > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... > chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:453: > if ([link length] != 0) { > Ditto. Hello Alexei, The link length would be 0 only in the case of an error (i.e. the call to get the link string returns nothing). If something goes wrong and the call returns nil, stringByAppendingString: will throw an exception, so that's the only reason for the check. But I'm not familiar enough with the code that manages the strings to know whether or not this would never happen. Best, __jayson
https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/excl... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/excl... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:241: [(HyperlinkTextView*)exitLabel_.get() Can the type of exitLabel be changed to the smart pointer of HyperlinkTextView, so you don't need these casts? https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm (right): https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:447: if ([link length] != 0) { On 2015/01/23 18:41:02, Alexei Svitkine wrote: > When would the link length be 0? I don't think this error handling is necessary - I don't think we do it anywhere else. If there is actually a problem, we'll see it in crash reports and can address it - but until then I'd keep the code simpler.
On 2015/01/23 19:14:19, Alexei Svitkine wrote: > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/excl... > File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm > (right): > > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/excl... > chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:241: > [(HyperlinkTextView*)exitLabel_.get() > Can the type of exitLabel be changed to the smart pointer of HyperlinkTextView, > so you don't need these casts? > > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... > File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm > (right): > > https://codereview.chromium.org/865373002/diff/1/chrome/browser/ui/cocoa/exte... > chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:447: > if ([link length] != 0) { > On 2015/01/23 18:41:02, Alexei Svitkine wrote: > > When would the link length be 0? > > I don't think this error handling is necessary - I don't think we do it anywhere > else. If there is actually a problem, we'll see it in crash reports and can > address it - but until then I'd keep the code simpler. No problem - I will remove the checks. Re: the exitLabel cast, it looks like changing its type to HyperlinkTextView works OK so I will include that change in my resubmit. Best, __jayson
PTAL - thanks!
Please add a TEST= line to the description that describes how someone can verify the changes are working as expected. Thanks! https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm (right): https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:94: if ([nsLinkText length] != 0) { You don't need this if either. https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:229: l10n_util::GetNSStringF( Please make a local var for this part. https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:240: if ([exitLinkText length] != 0) { No if?
On 2015/01/26 18:11:19, Alexei Svitkine wrote: > Please add a TEST= line to the description that describes how someone can verify > the changes are working as expected. Thanks! By a "TEST=" line do you mean a line that specifies the name of a test that can be run to verify the change? Can you point me at another issue that includes a TEST= line so that I can be sure to get the format right? Re: verifying the changes, I had put together some unit tests, but as they are based on the old API and the old API will eventually be removed, these new unit tests will stop functioning. I asked groby@chromium.org about this and she felt that writing unit tests for these changes was overkill and that they could be verified visually, which I also did. Please let me know how you would like me to proceed. > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:94: if > ([nsLinkText length] != 0) { > You don't need this if either. Sorry I missed this one. > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:229: > l10n_util::GetNSStringF( > Please make a local var for this part. Just to be clear, you are asking me to store the result of l10n_util::GetNSStringF(IDS_EXIT_FULLSCREEN_MODE_ACCELERATOR, l10n_util::GetStringUTF16(IDS_APP_ESC_KEY)) in a local variable? Can I ask why? The result of this call is not used anywhere else in the method. > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:240: if > ([exitLinkText length] != 0) { > No if? No, this check is needed - the previous if/else clause sets exitLinkText to either the empty string or a non zero-length string.
On 2015/01/26 18:42:21, shrike wrote: > On 2015/01/26 18:11:19, Alexei Svitkine wrote: > > Please add a TEST= line to the description that describes how someone can > verify > > the changes are working as expected. Thanks! > > By a "TEST=" line do you mean a line that specifies the name of a test that can > be run to verify the change? Can you point me at another issue that includes a > TEST= line so that I can be sure to get the format right? > > Re: verifying the changes, I had put together some unit tests, but as they are > based on the old API and the old API will eventually be removed, these new unit > tests will stop functioning. I asked mailto:groby@chromium.org about this and she felt > that writing unit tests for these changes was overkill and that they could be > verified visually, which I also did. Please let me know how you would like me to > proceed. > The TEST= line should specify how QA can test the feature manually, for use visible changes. For example, see: https://codereview.chromium.org/660813002 > > > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:94: if > > ([nsLinkText length] != 0) { > > You don't need this if either. > > Sorry I missed this one. > > > > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:229: > > l10n_util::GetNSStringF( > > Please make a local var for this part. > > Just to be clear, you are asking me to store the result of > l10n_util::GetNSStringF(IDS_EXIT_FULLSCREEN_MODE_ACCELERATOR, > l10n_util::GetStringUTF16(IDS_APP_ESC_KEY)) in a local variable? Can I ask why? > The result of this call is not used anywhere else in the method. Right. it would make the code more readable. > > > > https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:240: if > > ([exitLinkText length] != 0) { > > No if? > > No, this check is needed - the previous if/else clause sets exitLinkText to > either the empty string or a non zero-length string.
On 2015/01/26 20:06:22, Alexei Svitkine wrote: > The TEST= line should specify how QA can test the feature manually, for use > visible changes. For example, see: https://codereview.chromium.org/660813002 Hi Alexei, Thank you for that pointer. Do you have a suggestion for getting the ExclusiveAccessBubbleWindowController to display? I see that it's related to going into fullscreen mode but so far I haven't been able to figure out exactly which special invocation of fullscreen mode gets it to appear. Best, __jayson
On 2015/01/27 18:30:24, shrike wrote: > On 2015/01/26 20:06:22, Alexei Svitkine wrote: > > The TEST= line should specify how QA can test the feature manually, for use > > visible changes. For example, see: https://codereview.chromium.org/660813002 > > Hi Alexei, > > Thank you for that pointer. Do you have a suggestion for getting the > ExclusiveAccessBubbleWindowController to display? I see that it's related to > going into fullscreen mode but so far I haven't been able to figure out exactly > which special invocation of fullscreen mode gets it to appear. > > Best, > > __jayson It might have to be triggered by a site that triggers full-screen? Maybe try search for one of these on the web.
One more comment while you're still working on my previous ones. https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm (right): https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:454: chrome_style::GetLinkColor())]; Nit: This indent seems wrong. I think it should be indented 4 more spaces after linkColor:.
https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm (right): https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:454: chrome_style::GetLinkColor())]; On 2015/01/27 19:35:11, Alexei Svitkine wrote: > Nit: This indent seems wrong. I think it should be indented 4 more spaces after > linkColor:. You are referring to the chrome_style... line? I can indent it 3 more spaces, which will keep the line under 80 chars.
https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm (right): https://codereview.chromium.org/865373002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:454: chrome_style::GetLinkColor())]; On 2015/01/27 19:54:11, shrike wrote: > On 2015/01/27 19:35:11, Alexei Svitkine wrote: > > Nit: This indent seems wrong. I think it should be indented 4 more spaces > after > > linkColor:. > > You are referring to the chrome_style... line? I can indent it 3 more spaces, > which will keep the line under 80 chars. No, I mean indent less. Align with "gfx::SkColor.." then indent 4 more.
On 2015/01/27 19:26:38, Alexei Svitkine wrote: > It might have to be triggered by a site that triggers full-screen? Maybe try > search for one of these on the web. I found one such site, but it doesn't hit this piece of code. In just about all my attempts I've gotten a bubble window with some text and two buttons, one that exits fullscreen and one that allows it to continue. This doesn't match at all this section of code which embeds a single clickable option as a link within the text message.
On 2015/01/27 19:55:42, Alexei Svitkine wrote: > > You are referring to the chrome_style... line? I can indent it 3 more spaces, > > which will keep the line under 80 chars. > > No, I mean indent less. Align with "gfx::SkColor.." then indent 4 more. Oh, I see. Done.
Doesn't look like you uploaded a new patchset.
On 2015/01/27 20:17:27, Alexei Svitkine wrote: > Doesn't look like you uploaded a new patchset. No, I haven't. Were you expecting I had because I said "done?" I'm still getting the hang of the bug system. I made the change in my branch but haven't uploaded it yet. I probably won't don't anything until I'm finished rounding up all the TEST instructions.
Ah ok. Usually saying "done" means there's a new patch ready to take a look at. On Jan 27, 2015 3:20 PM, <shrike@chromium.org> wrote: > On 2015/01/27 20:17:27, Alexei Svitkine wrote: > >> Doesn't look like you uploaded a new patchset. >> > > No, I haven't. Were you expecting I had because I said "done?" I'm still > getting > the hang of the bug system. I made the change in my branch but haven't > uploaded > it yet. I probably won't don't anything until I'm finished rounding up all > the > TEST instructions. > > > https://codereview.chromium.org/865373002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/27 20:25:49, Alexei Svitkine wrote: > Ah ok. Usually saying "done" means there's a new patch ready to take a look > at. My apologies.... In any case, changes have been now been uploaded, and I've added the TEST info you requested. PTAL, and thank you.
lgtm with a couple last comments https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm (right): https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:85: NSString* nsLinkText = SysUTF16ToNSString(linkText); Use GetNSStringWithFixup instead of first getting the string16 and then converting it yourself for both of these. https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:232: messageText]; Nit: how about this formatting: exitLinkedText = [NSString stringWithFormat:@"%@ %@", exitLinkText, messageText];
https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm (right): https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:85: NSString* nsLinkText = SysUTF16ToNSString(linkText); On 2015/01/28 14:55:03, Alexei Svitkine wrote: > Use GetNSStringWithFixup instead of first getting the string16 and then > converting it yourself for both of these. That's a handy function, but the promoText = l10n_util::GetStringFUTF16() call also replaces a placeholder string sequence with linkText, and linkText has to be passed in as a base::string16. The call to GetNSStringWithFixup does some fixup on the string, but I don't see a version that also replaces placeholder string sequences? https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:232: messageText]; On 2015/01/28 14:55:03, Alexei Svitkine wrote: > Nit: how about this formatting: > > exitLinkedText = > [NSString stringWithFormat:@"%@ %@", exitLinkText, messageText]; Done.
https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm (right): https://codereview.chromium.org/865373002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller.mm:85: NSString* nsLinkText = SysUTF16ToNSString(linkText); On 2015/01/28 17:35:04, shrike wrote: > On 2015/01/28 14:55:03, Alexei Svitkine wrote: > > Use GetNSStringWithFixup instead of first getting the string16 and then > > converting it yourself for both of these. > > That's a handy function, but the promoText = l10n_util::GetStringFUTF16() call > also replaces a placeholder string sequence with linkText, and linkText has to > be passed in as a base::string16. The call to GetNSStringWithFixup does some > fixup on the string, but I don't see a version that also replaces placeholder > string sequences? Actually, I think the *Fixup variants aren't needed here - so you should be able to use GetNSString() and GetNSStringF() for the formatting.
On 2015/01/28 17:36:58, Alexei Svitkine wrote: > Actually, I think the *Fixup variants aren't needed here - so you should be able > to use GetNSString() and GetNSStringF() for the formatting. The string IDS_BOOKMARK_SYNC_PROMO_MESSAGE is defined as <ph name="SIGN_IN_LINK">$1<ex>Sign in</ex></ph> to get your bookmarks everywhere. l10n_util::GetStringFUTF16() replaces the <ph name="SIGN_IN_LINK">$1<ex>Sign in</ex></ph> portion with the base::string16 linkText. I don't see an equivalent variant of GetNSStringF() that replaces placeholder string sequences. If there is I can make the change you suggest, otherwise just swapping in GetStringFUTF16() will create a bug .
The CQ bit was checked by shrike@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865373002/60001
Okay, fair enough. On Wed, Jan 28, 2015 at 4:13 PM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/865373002/60001 > > > https://codereview.chromium.org/865373002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shrike@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865373002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/01/28 21:24:31, Alexei Svitkine wrote: > Okay, fair enough. Hi Alexei, I got a merge conflict from the commit-bot, and modified exclusive_access_bubble_window_controller_unittest.mm to fix it. Would you please take a look? Thank you.
lgtm
The CQ bit was checked by shrike@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865373002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f9bf8a81ff2ddd329846b2006185223799bc6bfb Cr-Commit-Position: refs/heads/master@{#313774} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
