I think we can punt these to a later phase. See  Issue 730958 .

+mathp fyi - I think we were chatting about this in a code review at some point.

Yes yes yes!!

What's up about this? Has someone started work on this?

I've looked at it briefly, but I decided to leave it for someone with more MacViews knowledge for the time being, and loop back later if nobody has touched it. It would be hugely helpful for our team.

Gotcha, thanks for the clarification :)

Yes, emphasis on huge help. Thanks Sarah for taking this on!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01b391917c9ff0150e303b4c5bf3ba34f1aaee15

commit 01b391917c9ff0150e303b4c5bf3ba34f1aaee15
Author: spqchan <spqchan@chromium.org>
Date: Mon Feb 05 23:16:53 2018

[MacViews] Wire up AutofillPopupView

Show toolkit-views AutofillPopupView with --secondary-ui-md.

Removed AutofillPopupBaseView FocusManager accelerator code.
The code is no longer necessary since it was added for a now
obsolete rAc dialog. The autofill popup will get dismissed by
the web contents, which listens for the accelerators.

Bug:  728182 
Change-Id: I9f7816fe41279a397f5066d28d88099d50ef033e
Reviewed-on: https://chromium-review.googlesource.com/889983
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534527}
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/BUILD.gn
[rename] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge_views.h
[rename] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge_views.mm
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/views/autofill/autofill_popup_base_view.h
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
[modify] https://crrev.com/01b391917c9ff0150e303b4c5bf3ba34f1aaee15/chrome/browser/ui/views/autofill/autofill_popup_view_views.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2

commit 3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2
Author: Sarah Chan <spqchan@chromium.org>
Date: Thu Feb 15 03:04:47 2018

Revert "[MacViews] Wire up AutofillPopupView"

This reverts commit 01b391917c9ff0150e303b4c5bf3ba34f1aaee15.

Reason for revert: Causes crash

Original change's description:
> [MacViews] Wire up AutofillPopupView
> 
> Show toolkit-views AutofillPopupView with --secondary-ui-md.
> 
> Removed AutofillPopupBaseView FocusManager accelerator code.
> The code is no longer necessary since it was added for a now
> obsolete rAc dialog. The autofill popup will get dismissed by
> the web contents, which listens for the accelerators.
> 
> Bug:  728182 
> Change-Id: I9f7816fe41279a397f5066d28d88099d50ef033e
> Reviewed-on: https://chromium-review.googlesource.com/889983
> Commit-Queue: Sarah Chan <spqchan@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534527}

TBR=ellyjones@chromium.org,tapted@chromium.org,estade@chromium.org,spqchan@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  728182 
Change-Id: Ibda0e0f4a15e54e2f2d4c19951719f7d604473ff
Reviewed-on: https://chromium-review.googlesource.com/919972
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536935}
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/BUILD.gn
[rename] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h
[rename] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/views/autofill/autofill_popup_base_view.h
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
[modify] https://crrev.com/3e4e3b3e2904bbdef7a2915bb11c3eaf7d2687b2/chrome/browser/ui/views/autofill/autofill_popup_view_views.h

spqchan: what happened to this after the revert? Can we get this in for M67?

I have a CL (https://chromium-review.googlesource.com/c/chromium/src/+/974106)
with a potential fix for the crash

The plan is to roll this out on a different flag

Seems like cl listed at #16 is going thru CQ now.

When do we expect Cl (https://chromium-review.googlesource.com/c/chromium/src/+/974106) to land?

Let's push this to M68

Sounds good - updating flags.

Thanks Sarah and Elly. FYI, M68 is also when we plan to release birthday improvements to the Autofill popup. Having MacViews at the same time would be ideal, so let us know if there's a chance it won't make it to M68. 

** Bulk Edit **

FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8775569edf869fd6e7912d9e4e7cbc9e70db636

commit b8775569edf869fd6e7912d9e4e7cbc9e70db636
Author: spqchan <spqchan@chromium.org>
Date: Thu Apr 05 00:46:09 2018

[MacViews] Wire up AutofillPopupView

Added a "MacViewsAutofillPopup" feature flag and
--mac-views-autofill-popup switch. If it's enabled,
show the Autofill popup view using toolkit instead of
cocoa.

Removed AutofillPopupBaseView FocusManager accelerator code.
The code is no longer necessary since it was added for a now
obsolete rAc dialog. The autofill popup will get dismissed by
the web contents, which listens for the accelerators.

This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/889983

The CL was reverted because of a crash (https://crbug.com/809830)
which was caused by the popup's container view's window set to
nil.

Bug:  728182 
Change-Id: I02f74b5627e6559c4e4dd77a35860e77d000a0d5
Reviewed-on: https://chromium-review.googlesource.com/974106
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548270}
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/about_flags.cc
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/ui/views/autofill/autofill_popup_base_view.h
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/components/autofill/core/browser/autofill_experiments.cc
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/components/autofill/core/browser/autofill_experiments.h
[modify] https://crrev.com/b8775569edf869fd6e7912d9e4e7cbc9e70db636/tools/metrics/histograms/enums.xml

Can this be marked as fixed if nothing else is pending?

Able to reproduce the issue on mac 10.13.3 using chrome version #64.0.3256.0

Verified the fix on Mac 10.13.3 using Chrome version #67.0.3389.0 as per the comment #23.
Attaching screen cast for reference.
Observed that "MacViewsAutofillPopup" feature flag is added in chrome://flags.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!

Deleted: 728182.mp4 740 KB

	728182.mp4 740 KB View Download