Opened 3 years ago

Closed 3 years ago

Last modified 16 months ago

#1672 closed change (fixed)

Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP

Reported by: sergz Assignee: sergz
Priority: P3 Milestone: Adblock-Plus-for-Internet-Explorer-1.6
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: eric@…, oleksandr, fhd Blocked By:
Blocking: #119, #1619 Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Rraceanu Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4937147073167360/

Description

Background

Developing injecting CSS I want to raise again the question about using ATL and in particular ATL::IDispEventImpl. First of all take a look at #1619 and current impl of `IDispatch::Invoke`.

  • wrong return type
  • we don't follow the convention to properly set outgoing parameters
  • we don't check the incoming parameters
  • a lot of dead code
  • ask yourself do you like it? Does it make sense to do it manually again (apparently with bugs) when it's already done?

Take a look at the proposed code review.
We don't have to think how to implement IDispatch::Invoke, the everything we need is to declare method-slot for the event and write our code inside it. BTW, in some issue such refactoring had been already mentioned.
I would say it's much easier to work with it. If we really need some specific functionality there we always can switch to our implementation.

As far as I remember the main argument was that ATL is not available in Express edition of Visual Studio, now there is Community edition which seems the replacement of Express edition for individual developers and this edition includes ATL, so no troubles with availability of ATL for others.

We might should concentrate more on fixing bugs, implementing functionality and switching to the recent version of tools (MSVS2013) instead of re-implementing ATL.

Change History (16)

comment:1 Changed 3 years ago by sergz

  • Cc eric@… oleksandr fhd trev added

comment:2 Changed 3 years ago by trev

  • Cc fhd trev removed

comment:3 Changed 3 years ago by trev

  • Cc fhd added

comment:4 follow-up: Changed 3 years ago by eric@…

From the MSDN documentation on ATL::IDispEventImpl http://msdn.microsoft.com/en-us/library/d7eecxta.aspx

This class and its members cannot be used in applications that execute in the Windows Runtime.

It seems that may well kill Windows Store compatibility.

comment:5 in reply to: ↑ 4 Changed 3 years ago by sergz

Replying to eric@…:

From the MSDN documentation on ATL::IDispEventImpl http://msdn.microsoft.com/en-us/library/d7eecxta.aspx

This class and its members cannot be used in applications that execute in the Windows Runtime.

It seems that may well kill Windows Store compatibility.

As I understand we don't compile it for Windows Runtime, do we? Anyway I would say we use quite a lot of functions which are not allowed there, for example API to work with pipes (in general IPC is a painful subject in Windows runtime). I'm not sure whether we will run this code under Windows Runtime and it seems better to use C++/CX for it. BTW, C++/CX provides another way to connect to events.

@oleksandr, could you check how it influences the Windows Store compatibility?

comment:6 Changed 3 years ago by oleksandr

  • Platform changed from Unknown to Internet Explorer
  • Priority changed from Unknown to P3

comment:7 Changed 3 years ago by oleksandr

We have not even planned to run in Windows Runtime. That would mean running for something like Internet Explorer "Modern UI" version. What we have planned is just submitting to Windows Store for distribution, but it would still be the same plugin we have now. So I don't think using IDispEventImpl would cause any more problems than we already have. We are using quite a lot of other "problematic" APIs anyway, as Sergei mentioned.

comment:8 Changed 3 years ago by eric@…

It appears we won't have a problem with this for IE11, the current version as of this writing. IE11 under Modern UI doesn't load extensions, so we're safe for now.

I have a lingering concern about IE12, since it's conceivable that we might end up running unwittingly under WinRT because of external actions by Microsoft.

comment:9 Changed 3 years ago by sergz

  • Owner set to sergz

comment:10 Changed 3 years ago by sergz

  • Status changed from new to reviewing

comment:11 Changed 3 years ago by sergz

  • Blocking 119 added

comment:12 Changed 3 years ago by oleksandr

  • Ready set

comment:13 Changed 3 years ago by oleksandr

  • Blocking 1619 added

comment:14 Changed 3 years ago by oleksandr

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 16 months ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-Next
  • Tester set to Unknown

comment:16 Changed 16 months ago by rraceanu

  • Tester changed from Unknown to Rraceanu
Note: See TracTickets for help on using tickets.