Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(318)

Issue 2815013: Refactor the translate infobars on mac to match the new windows code. (Closed)

Created:
10 years, 6 months ago by feldstein
Modified:
9 years, 7 months ago
Reviewers:
Jay Civelli, jeremy
CC:
chromium-reviews, John Grabowski, Paweł Hajdan Jr., ben+cc_chromium.org, pam+watch_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Refactor the translate infobars on mac to match the new windows code. Breaks it up into 4 different classes instead of 1 class full of switches and if statements. BUG=none TEST=unit_tests.TranslationInfoBarTest Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50641

Patch Set 1 #

Patch Set 2 : Remove unused observer #

Patch Set 3 : Some cleanup. #

Patch Set 4 : Style fixes #

Patch Set 5 : Remove old files #

Patch Set 6 : Synced, hopefully this compiles #

Patch Set 7 : Fix the delegate interface, i broke windows. #

Patch Set 8 : remove unnecessary imports #

Total comments: 15

Patch Set 9 : Fix delegate for other locales and make sure to use swapLanguages and numlabels properly #

Total comments: 6

Patch Set 10 : Fix test helpers and move into unittest file. #

Patch Set 11 : Add a comment #

Patch Set 12 : Move unittest stuff back to class files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1014 lines, -1162 lines) Patch
M chrome/browser/browser_main.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
A chrome/browser/cocoa/translate/after_translate_infobar_controller.h View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/after_translate_infobar_controller.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +59 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/before_translate_infobar_controller.h View 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/before_translate_infobar_controller.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +47 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/translate_infobar_base.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +143 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/translate_infobar_base.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +579 lines, -0 lines 0 comments Download
A + chrome/browser/cocoa/translate/translate_infobar_unittest.mm View 5 chunks +64 lines, -84 lines 0 comments Download
A chrome/browser/cocoa/translate/translate_message_infobar_controller.h View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
A chrome/browser/cocoa/translate/translate_message_infobar_controller.mm View 1 2 3 4 5 6 10 11 1 chunk +54 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/translate_infobar.h View 1 2 3 4 1 chunk +0 lines, -115 lines 0 comments Download
D chrome/browser/cocoa/translate_infobar.mm View 1 2 3 4 5 1 chunk +0 lines, -941 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate2.h View 1 2 3 4 5 6 3 chunks +12 lines, -11 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate2.cc View 1 2 3 4 5 6 7 8 9 3 chunks +10 lines, -6 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -2 lines 0 comments Download
M chrome/chrome_tests.gypi View 9 10 11 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 9 (0 generated)
feldstein
Hey guy, can you take a look?
10 years, 6 months ago (2010-06-18 03:18:18 UTC) #1
Jay Civelli
On 2010/06/18 03:18:18, feldstein wrote: > Hey guy, can you take a look? I am ...
10 years, 6 months ago (2010-06-18 20:55:14 UTC) #2
jeremy
http://codereview.chromium.org/2815013/diff/2002/18003 File chrome/browser/cocoa/translate/after_translate_infobar_controller.mm (right): http://codereview.chromium.org/2815013/diff/2002/18003#newcode16 chrome/browser/cocoa/translate/after_translate_infobar_controller.mm:16: &strings, &swapLanguages); Should probably use swapLanguages? http://codereview.chromium.org/2815013/diff/2002/18003#newcode34 chrome/browser/cocoa/translate/after_translate_infobar_controller.mm:34: if ...
10 years, 6 months ago (2010-06-20 04:30:48 UTC) #3
feldstein
Jay, i fixed up the delegate so it works when translating in other languages please ...
10 years, 6 months ago (2010-06-22 00:05:35 UTC) #4
jeremy
http://codereview.chromium.org/2815013/diff/2002/18003 File chrome/browser/cocoa/translate/after_translate_infobar_controller.mm (right): http://codereview.chromium.org/2815013/diff/2002/18003#newcode43 chrome/browser/cocoa/translate/after_translate_infobar_controller.mm:43: - (NSArray*)visibleControls { Nope, nothing special that I know ...
10 years, 6 months ago (2010-06-22 11:59:39 UTC) #5
Jay Civelli
LGTM for the delegate change (thanks for catching this bug!)
10 years, 6 months ago (2010-06-22 16:40:29 UTC) #6
feldstein
PTAL, i moved the unit test stuff into the unit test file, and also fixed ...
10 years, 6 months ago (2010-06-22 20:27:39 UTC) #7
jeremy
LGTM I don't see the update to the _unittest file in the patch? if the ...
10 years, 6 months ago (2010-06-23 03:28:26 UTC) #8
feldstein
10 years, 6 months ago (2010-06-23 19:33:30 UTC) #9
On 2010/06/23 03:28:26, jeremy wrote:
> LGTM
> 
> I don't see the update to the _unittest file in the patch?
> 
> if the UNIT_TEST macro isn't defined for the .mm files, that sounds like a
bug.
> Could you take a quick look and if it isn't something triival could you file a
> bug.
> 
> Also, on second thoughts moving the test code for each class into categories
in
> the unittest file is a bad idea, could you move it back into the individual
> files in the patch and we can #ifdef it out for shipping code once the above
> issue is fixed.  Sorry for the trouble.

I talked to mentovai about this, UNIT_TEST is never set to true when building
anything but the unit test project.  The actual infobar code is all built into a
library that the unit test code links to. There's really no good way to do this
so I'm just going to leave it as it was (same as all the other cocoa classes.)

I added the missing unittest file, thanks for catching that.

Powered by Google App Engine
This is Rietveld 408576698