Non-V8 build doesn't support V8EnabledPerContext. And it will access a null pointer in DOMWindowPagePopup::pagePopupController rather than returning "undefined"
Created attachment 161536 [details] the patch
Ping Chromium team: any objections?
Comment on attachment 161536 [details] the patch I believe this is needed to implement some <input> elements, like the date picker. We should just fix the underlying crash.
I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects. Anyway, the right fix is to add support of EnabledPerContext to JSC binding.
(In reply to comment #4) > I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects. > > Anyway, the right fix is to add support of EnabledPerContext to JSC binding. Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property?
(In reply to comment #5) > (In reply to comment #4) > > I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects. > > > > Anyway, the right fix is to add support of EnabledPerContext to JSC binding. > > Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property? Why do you need to make it V8-only? If you need no window.pagePopupController, you should exclude DOMWindowPagePopup.idl from build target.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects. > > > > > > Anyway, the right fix is to add support of EnabledPerContext to JSC binding. > > > > Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property? > > Why do you need to make it V8-only? Because it crashes on a non-V8 build. > If you need no window.pagePopupController, you should exclude DOMWindowPagePopup.idl from build target. DOMWindowPagePopup.idl is included in WebCore/CMakeLists.txt, which is a cross-platform file. I don't prefer to add platform-specific code to that cmake file. Any other solution?
Why not just disable PAGE_POPUP in your port?
(In reply to comment #8) > Why not just disable PAGE_POPUP in your port? Because we are using ENABLE(PAGE_POPUP)
I don't think ENABLE(PAGE_POPUP) works correctly without DOMWindowPagePopup.idl and EnabledPerContext. It seems that the proper resolution is to finish your implementation of ENABLE(PAGE_POPUP) by implementing EnabledPerContext.
(In reply to comment #10) > I don't think ENABLE(PAGE_POPUP) works correctly without DOMWindowPagePopup.idl and EnabledPerContext. It seems that the proper resolution is to finish your implementation of ENABLE(PAGE_POPUP) by implementing EnabledPerContext. What do you mean by "correctly"? We've got it work as we want, until it is broken by r118482. I think you guys should implement EnabledPerContext before including DOMWindowPagePopup.idl into the cross-platform cmake file.
> What do you mean by "correctly"? The pagePopupController object should be exposed to page popups, but not to arbitrary web sites. For that to work, it needs to be enabled per context. Simply ifdefing DOMWindowPagePopup out of the build will break ENABLE(PAGE_POPUP). It has the same effect as simply disabling PAGE_POPUP.
PAGE_POPUP was removed.