In the current implementation of the AddHandler method within CefMessageRouterBrowserSideImpl, it appears that the bool first parameter is not having the intended effect due to the data structure used to store handlers.
///
/// Add a new query handler. If |first| is true it will be added as the first
/// handler, otherwise it will be added as the last handler. Returns true if
/// the handler is added successfully or false if the handler has already been
/// added. Must be called on the browser process UI thread. The Handler object
/// must either outlive the router or be removed before deletion.
///
virtual bool AddHandler(Handler* handler, bool first) = 0;
The issue arises from the fact that handler_set_ is declared as a std::set<Handler*>. In a std::set, elements are sorted and ordered by the values they point to, not by the order of insertion or any custom criteria like bool first would suggest.
Unit tests coverage
It is important to note that there are existing unit tests that should cover this functionality. These unit tests can be found in the CEF source code:
void AddHandlers(
CefRefPtr<CefMessageRouterBrowserSide> message_router) override {
// OnQuery call order will verify that the first/last ordering works as
// expected.
EXPECT_TRUE(message_router->AddHandler(&handler1_, true));
EXPECT_TRUE(message_router->AddHandler(&handler0_, true));
EXPECT_TRUE(message_router->AddHandler(&handler2_, false));
// Can't add the same handler multiple times.
EXPECT_FALSE(message_router->AddHandler(&handler1_, true));
}
The unit tests verify the functionality of AddHandler, but they currently work correctly because the pointers to handlers have increasing values. This approach is not a reliable way to ensure the expected ordering behavior and does not follow the documented behavior of the AddHandler method.
Expected behavior
In the AddHandler method, when the bool first parameter is true, the handler should be inserted at the beginning of the set, and when false, it should be inserted at the end. However, due to the nature of std::set, this is not happening as expected.
Proposed Solution:
To address this issue, it may be necessary to change the underlying data structure used for storing handlers in the CefMessageRouterBrowserSide or introduce an alternative mechanism that can maintain the desired order of handlers based on the bool first parameter.
Another approach could be to remove bool first from the API, or change API description to mention that parameter is ignored.
Description
In the current implementation of the
AddHandler
method withinCefMessageRouterBrowserSideImpl
, it appears that thebool first
parameter is not having the intended effect due to the data structure used to store handlers.Here is AddHandler description:
Here is the AddHandler implementation:
The issue arises from the fact that
handler_set_
is declared as astd::set<Handler*>
. In astd::set
, elements are sorted and ordered by the values they point to, not by the order of insertion or any custom criteria likebool first
would suggest.Unit tests coverage
It is important to note that there are existing unit tests that should cover this functionality. These unit tests can be found in the CEF source code:
The unit tests verify the functionality of AddHandler, but they currently work correctly because the pointers to handlers have increasing values. This approach is not a reliable way to ensure the expected ordering behavior and does not follow the documented behavior of the AddHandler method.
Expected behavior
In the
AddHandler
method, when thebool first
parameter istrue
, the handler should be inserted at the beginning of the set, and whenfalse
, it should be inserted at the end. However, due to the nature ofstd::set
, this is not happening as expected.Proposed Solution:
To address this issue, it may be necessary to change the underlying data structure used for storing handlers in the
CefMessageRouterBrowserSide
or introduce an alternative mechanism that can maintain the desired order of handlers based on thebool first
parameter. Another approach could be to removebool first
from the API, or change API description to mention that parameter is ignored.