Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563863003:
[Autofill] Remove AutofillPopupController::IsWarning() (Closed)
|
Created:
4 years ago by dhna Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, mac-reviews_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Remove AutofillPopupController::IsWarning()
Replace it by GetValueFontColorForRow()
BUG=666189
TEST=existing
Committed: https://crrev.com/29a218ca46ffa933ed20f6e4ea961fac2564f88a
Cr-Commit-Position: refs/heads/master@{#438879}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 1
Patch Set 4 : fix #
Total comments: 2
Patch Set 5 : Remove unnecessary code. #
Total comments: 1
Patch Set 6 : fix #
Total comments: 2
Patch Set 7 : fix #
Messages
Total messages: 36 (16 generated)
corona10@gmail.com changed reviewers: + rsesek@chromium.org
rsesek@ PTAL
I don't think this change is correct. GetValueFontColorForRow returns an SkColor.
On 2016/12/12 17:18:03, Robert Sesek wrote: > I don't think this change is correct. GetValueFontColorForRow returns an > SkColor. rsesek@ Sorry. It did not read it carefully. I fixed it Can you take a look?
On 2016/12/13 02:12:47, dhna wrote: > On 2016/12/12 17:18:03, Robert Sesek wrote: > > I don't think this change is correct. GetValueFontColorForRow returns an > > SkColor. > > rsesek@ > Sorry. It did not read it carefully. > I fixed it Can you take a look? rsesek@ Sorry. I did not read it carefully. I fixed it Can you take a look?
Description was changed from ========== Remove AutofillPopupController::IsWarning() BUG=666189 ========== to ========== Remove AutofillPopupController::IsWarning() And replace it by GetValueFontColorForRow() BUG=666189 ==========
rsesek@chromium.org changed reviewers: + mathp@chromium.org
+mathp who's CC'd on the bug I still don't know if this is correct. Shouldn't we be using the colors returned by GetValueFontColorForRow?
https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: NSColor* nameColor = nil; Have a look at https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... and repeat the pattern, please! Thanks
On 2016/12/13 15:45:10, Mathieu Perreault wrote: > https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): > > https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: NSColor* > nameColor = nil; > Have a look at > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... > and repeat the pattern, please! > > Thanks mathp@ Done. As your comment. But it will be not problem after this CL? By using skia::SkColorToSRGBNSColor() NSColor will be same in case of PAYMENT_DISABLED_MESSAGE(= warningColor) and default(= nameColor). but it will has different color in case of POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE. Before this CL, PAYMENT_DISABLED_MESSAGE and SECURE_WARNING_MESSAGE has same color.
On 2016/12/13 17:07:32, dhna wrote: > On 2016/12/13 15:45:10, Mathieu Perreault wrote: > > > https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): > > > > > https://codereview.chromium.org/2563863003/diff/40001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: NSColor* > > nameColor = nil; > > Have a look at > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... > > and repeat the pattern, please! > > > > Thanks > > mathp@ > Done. As your comment. > But it will be not problem after this CL? > By using skia::SkColorToSRGBNSColor() > NSColor will be same in case of PAYMENT_DISABLED_MESSAGE(= warningColor) and > default(= nameColor). > but it will has different color in case of > POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE. > Before this CL, PAYMENT_DISABLED_MESSAGE and SECURE_WARNING_MESSAGE has same > color. Your change is an improvement in regards to the consistency, since all views implementation will now call GetValueFontColorForRow.
Some comments https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (left): https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: // IsWarning() method. This has not been addressed. Please remove IsWarning from autofill_popup_controller https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:204: controller_->IsWarning(index) ? [self warningColor] : [self nameColor]; can we get rid of the warningColor definition on the object now?
On 2016/12/13 17:57:12, Mathieu Perreault wrote: > Some comments > > https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (left): > > https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: // > IsWarning() method. > This has not been addressed. Please remove IsWarning from > autofill_popup_controller > > https://codereview.chromium.org/2563863003/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:204: > controller_->IsWarning(index) ? [self warningColor] : [self nameColor]; > can we get rid of the warningColor definition on the object now? mathp@ Thanks I understand what you 're saying. i remove unnecessary code. PTAL.
Thanks https://codereview.chromium.org/2563863003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (left): https://codereview.chromium.org/2563863003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:27: - (NSColor*)warningColor; What about https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... ?
On 2016/12/14 00:25:26, Mathieu Perreault wrote: > Thanks > > https://codereview.chromium.org/2563863003/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (left): > > https://codereview.chromium.org/2563863003/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:27: - > (NSColor*)warningColor; > What about > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... > ? Done. PTAL
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove AutofillPopupController::IsWarning() And replace it by GetValueFontColorForRow() BUG=666189 ========== to ========== [Autofill] Remove AutofillPopupController::IsWarning() Replace it by GetValueFontColorForRow() BUG=666189 TEST=existing ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:19: #include "ui/gfx/color_palette.h" Unused include? https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: NSColor* nameColor = skia::SkColorToSRGBNSColor(skColor); Since skColor is only used here you could just do: skia::SkColorToSRGBNSColor(controller_->layout_model().GetValueFontColorForRow(index));
On 2016/12/14 20:14:21, Robert Sesek wrote: > https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): > > https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:19: #include > "ui/gfx/color_palette.h" > Unused include? > > https://codereview.chromium.org/2563863003/diff/100001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:202: NSColor* > nameColor = skia::SkColorToSRGBNSColor(skColor); > Since skColor is only used here you could just do: > skia::SkColorToSRGBNSColor(controller_->layout_model().GetValueFontColorForRow(index)); rsesek@ Thanks~! I fixed it PTAL.
lgtm
The CQ bit was checked by corona10@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
On 2016/12/15 17:24:23, Robert Sesek wrote: > LGTM Thanks!!
The CQ bit was checked by corona10@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481826971629820,
"parent_rev": "1b3fd1e66e2a52c0a8b532c21da101ee89de9095", "commit_rev":
"91c748335e16aac120484321a3cb5f64186c8f62"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Remove AutofillPopupController::IsWarning() Replace it by GetValueFontColorForRow() BUG=666189 TEST=existing ========== to ========== [Autofill] Remove AutofillPopupController::IsWarning() Replace it by GetValueFontColorForRow() BUG=666189 TEST=existing Review-Url: https://codereview.chromium.org/2563863003 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Remove AutofillPopupController::IsWarning() Replace it by GetValueFontColorForRow() BUG=666189 TEST=existing Review-Url: https://codereview.chromium.org/2563863003 ========== to ========== [Autofill] Remove AutofillPopupController::IsWarning() Replace it by GetValueFontColorForRow() BUG=666189 TEST=existing Committed: https://crrev.com/29a218ca46ffa933ed20f6e4ea961fac2564f88a Cr-Commit-Position: refs/heads/master@{#438879} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/29a218ca46ffa933ed20f6e4ea961fac2564f88a Cr-Commit-Position: refs/heads/master@{#438879} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
