Allow synchronously sending extra_info to renderer at each browser creation

#147 Declined
Repository
cef-ms
Branch
issue_1088
Repository
chromiumembedded
Branch
master
Author
  1. cef-ms
Reviewers
Description

Hi,

this PR aims to implement the feature from issue #1088. The only thing I couldn't get properly working is the proposed wrapper object for common arguments of CreateBrowser/CreateBrowserView/OnBeforePopup. I always ran into ownership problems when tried to pass it through the C API surface, so I'm open to suggestions regarding this.

--

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

Comments (25)

  1. Marshall Greenblatt

    This PR needs to be updated (or recreated) to fix merge conflicts with current master.

  2. Dmitry Azaraev

    From top-level API perspective I'm prefer to have CefDictionaryValue instead of CefListValue: list value provoke users use naive way with hardcoded indexes, just like in tests. Add fact that creating info and consuming is very far from each other - is very easy to get errors.

    Surely we can put dictionary in zero slot of list and use dictionary or whatever, bad this only workaround/pattern around single parameter. Normally code should use internal type with serializers, but again pattern.

    CefDictionaryValue actually can be wrapped to list at top level APIs and unwrapped for callback if it simplier than change extra_info everywhere.

    So, i propose publish CefDictionaryValue as extra info, just to get whole API bit more fool-proof. (But not a big deal generally.)

  3. cef-ms author

    I rebased the PR on top of master. Also the above comments made sense so I changed the extra info parameter to be a dictionary.

  4. cef-ms author

    Thanks for reviewing! I fixed the issues mentioned in the comments and rebased on top of master.

  5. Marshall Greenblatt

    Please re-run the translator tool and fix_style tool for this PR. Also see comments inline.

  6. cef-ms author

    Is there anything else to address? Can you give a rough estimate on when this can be merged?

    Thank you!

  7. Marshall Greenblatt

    Please don’t include style fixes for source files unrelated to your change. Also see comments inline.

    1. Marshall Greenblatt

      Please reply “Done” or similar to comments as you address them. It makes it easier to review future versions.

  8. Marshall Greenblatt

    Please update this PR to resolve conflicts. Sorry for the long delay in merging.

  9. Marshall Greenblatt

    Thanks for the update. Please see the new comment inline. Also, please remove the disclaimer text from your commit message – such text cannot be accepted by this project.

  10. Marshall Greenblatt

    Thanks, this PR looks good except for a minor style issue commented inline. I’ll test and merge this PR after the pending Chromium update lands.